Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 33 additions & 34 deletions python/mdvtools/dbutils/safe_mdv_app.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,63 +7,62 @@

logger.info("safe_mdv_app.py starting")

# https://stackoverflow.com/a/36033627/279703
class PrefixMiddleware(object):
def __init__(self, app, prefix=''):
self.app = app
self.prefix = prefix

def __call__(self, environ, start_response):
if environ['PATH_INFO'].startswith(self.prefix):
environ['PATH_INFO'] = environ['PATH_INFO'][len(self.prefix):]
environ['SCRIPT_NAME'] = self.prefix
return self.app(environ, start_response)
else:
response_body = b"This url does not belong to the app."
headers = [('Content-Type', 'text/plain'), ('Content-Length', str(len(response_body)))]
start_response('404 Not Found', headers)
return [response_body]

# Import the app at the module level (outside any conditionals)
try:
from mdvtools.dbutils.mdv_server_app import app
# app = socketio
if app is None:
logger.error("app from mdv_server_app is None - using fallback")
app = Flask(__name__)
except Exception as e:
logger.exception(f'Error importing mdv_app: {e}')
app = Flask(__name__)

# Apply prefix middleware at module level - works for both direct run and WSGI deployment
prefix = os.environ.get('MDV_API_ROOT', '')
if prefix:
# Remove trailing slash from prefix if it exists
if prefix.endswith('/'):
prefix = prefix[:-1]

logger.info(f"Applying PrefixMiddleware with prefix: {prefix}")
app.wsgi_app = PrefixMiddleware(app.wsgi_app, prefix=prefix)
Comment on lines +37 to +45

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Reapply PrefixMiddleware after setting default MDV_API_ROOT

When the module loads, we capture MDV_API_ROOT and (optionally) wrap app.wsgi_app. Later, inside the __main__ block, we set the default /test/ value if the variable was absent. Because the middleware was already skipped earlier, we never re-wrap the app after this assignment. As a result, running python safe_mdv_app.py with no pre-set env var leaves the server listening on the root path while the rest of the stack believes it’s under /test/, recreating the socket path mismatch we’re trying to solve.

Please move the prefix application into a helper that can be invoked both at module load and after setting the default, e.g.:

+def apply_prefix_middleware() -> None:
+    prefix = os.environ.get('MDV_API_ROOT', '')
+    if prefix.endswith('/'):
+        prefix = prefix[:-1]
+    if prefix and not isinstance(app.wsgi_app, PrefixMiddleware):
+        logger.info(f"Applying PrefixMiddleware with prefix: {prefix}")
+        app.wsgi_app = PrefixMiddleware(app.wsgi_app, prefix=prefix)
+
+apply_prefix_middleware()
...
     # Set default for local testing if not already set
     if 'MDV_API_ROOT' not in os.environ:
         os.environ['MDV_API_ROOT'] = '/test/'
+        apply_prefix_middleware()

This keeps the import-time behavior for deployments while ensuring the local default actually takes effect.

Also applies to: 58-66

🤖 Prompt for AI Agents
In python/mdvtools/dbutils/safe_mdv_app.py around lines 37-45 and 58-66, the
PrefixMiddleware is applied at import time using MDV_API_ROOT but later a
default is set in __main__, so when MDV_API_ROOT is absent the app never gets
wrapped; refactor by extracting the prefix-detection-and-application logic into
a small helper function (e.g., apply_prefix_middleware(app)) that reads
MDV_API_ROOT, normalizes it (remove trailing slash), logs, and wraps
app.wsgi_app only when non-empty, then call that helper both at module load (to
preserve deployment behavior) and again after setting the default in the
__main__ block so the default `/test/` will be applied when running the module
directly.


# Keep the existing __main__ block for direct execution
if __name__ == '__main__':
"""
Running the app in local debugger with a path prefix
This can be usefull for testing behaviour app running on a sub-path
This can be useful for testing behaviour app running on a sub-path
"""
from mdvtools.websocket import socketio
from mdvtools.socketio_upload import initialize_socketio_upload
# https://stackoverflow.com/a/36033627/279703
class PrefixMiddleware(object):

def __init__(self, app, prefix=''):
self.app = app
self.prefix = prefix

def __call__(self, environ, start_response):
if environ['PATH_INFO'].startswith(self.prefix):
environ['PATH_INFO'] = environ['PATH_INFO'][len(self.prefix):]
environ['SCRIPT_NAME'] = self.prefix
return self.app(environ, start_response)
else:
# It's good practice to ensure the Content-Length header is set.
# The Werkzeug BaseResponse object (which Flask uses) handles this automatically
# if the response is a string or a list of strings, but here we are returning a list of bytes.
response_body = b"This url does not belong to the app."
headers = [('Content-Type', 'text/plain'), ('Content-Length', str(len(response_body)))]
start_response('404 Not Found', headers)
return [response_body]

logger.info("running as __main__")
os.environ['MDV_API_ROOT'] = '/test/'

# Get the prefix from the environment variable
prefix = os.environ.get('MDV_API_ROOT', '')
# Remove trailing slash from prefix if it exists, as PATH_INFO usually starts with a slash
if prefix.endswith('/'):
prefix = prefix[:-1]

if prefix:
logger.info(f"Applying PrefixMiddleware with prefix: {prefix}")
app.wsgi_app = PrefixMiddleware(app.wsgi_app, prefix=prefix)
# Set default for local testing if not already set
if 'MDV_API_ROOT' not in os.environ:
os.environ['MDV_API_ROOT'] = '/test/'

# Register upload socket events
if initialize_socketio_upload is not None:
initialize_socketio_upload(app)
# app.run(host='0.0.0.0', debug=False, port=5056)

# Run with socketio if available, otherwise fallback to Flask
if socketio is not None:
socketio.run(app, host='0.0.0.0', debug=False, port=5056)
else:
Expand Down
13 changes: 10 additions & 3 deletions python/mdvtools/websocket.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class SocketIOContextRequest(FlaskRequest):
def mdv_socketio(app: Flask):
"""
Experimental and not to be trusted pending design work etc.

Do we have a SocketIO for the entire app and route messages internally,
- yes probably.
What `path` should it use? We'll need to make sure it is compatible with
Expand All @@ -40,8 +40,15 @@ def mdv_socketio(app: Flask):
global socketio
# allow cors for localhost:5170-5179
# cors = [f"http://localhost:{i}" for i in range(5170,5180)]
socketio = SocketIO(app, cors_allowed_origins="*")
log("socketio initialized with cors_allowed_origins wildcard")

# Get the API root path for SocketIO path configuration
api_root = app.config.get('mdv_api_root', '/')
if api_root != '/' and not api_root.endswith('/'):
api_root += '/'
socketio_path = f"{api_root}socket.io"

socketio = SocketIO(app, cors_allowed_origins="*", path=socketio_path)
log(f"socketio initialized with cors_allowed_origins wildcard and path={socketio_path}")
Comment on lines +50 to +51

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid wildcard CORS in production; make origins configurable.

Using cors_allowed_origins="" is a security posture gap. Read allowed origins from config and fall back to "" only in dev/test.

Apply this diff:

-    socketio = SocketIO(app, cors_allowed_origins="*", path=socketio_path)
-    log(f"socketio initialized with cors_allowed_origins wildcard and path={socketio_path}")
+    origins_cfg = app.config.get("MDV_CORS_ALLOWED_ORIGINS") or app.config.get("mdv_cors_allowed_origins")
+    if isinstance(origins_cfg, str):
+        allowed_origins = [o.strip() for o in origins_cfg.split(",") if o.strip()]
+    else:
+        allowed_origins = origins_cfg
+    if not allowed_origins:
+        allowed_origins = "*" if (app.debug or app.testing) else []
+    socketio = SocketIO(app, cors_allowed_origins=allowed_origins, path=socketio_path)
+    log(f"socketio initialized path={socketio_path} cors={allowed_origins}")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
socketio = SocketIO(app, cors_allowed_origins="*", path=socketio_path)
log(f"socketio initialized with cors_allowed_origins wildcard and path={socketio_path}")
origins_cfg = app.config.get("MDV_CORS_ALLOWED_ORIGINS") or app.config.get("mdv_cors_allowed_origins")
if isinstance(origins_cfg, str):
allowed_origins = [o.strip() for o in origins_cfg.split(",") if o.strip()]
else:
allowed_origins = origins_cfg
if not allowed_origins:
allowed_origins = "*" if (app.debug or app.testing) else []
socketio = SocketIO(app, cors_allowed_origins=allowed_origins, path=socketio_path)
log(f"socketio initialized path={socketio_path} cors={allowed_origins}")


# @socketio.on("message")
# def message(data):
Expand Down
4 changes: 2 additions & 2 deletions src/charts/dialogs/SocketIOUploadClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,10 +217,10 @@ export class SocketIOUploadClient {
this.socket = io(`${this.config.namespace}`, {
path: this.config.socketPath || '/socket.io', // '/test/socket.io' or '/carroll/socket.io'
autoConnect: false,
transports: ['polling'],
transports: ['websocket', 'polling'],
timeout: 60000,
forceNew: true,
reconnection: false,
reconnection: true,
reconnectionDelay: 1000,
reconnectionAttempts: 5,
});
Expand Down