Skip to content

Update server.py - #99

Merged
cj-vana merged 1 commit into
betafrom
new-pr
Sep 22, 2025
Merged

cj-vana merged 1 commit into
betafrom
new-pr

Conversation

@cj-vana

@cj-vana cj-vana commented Sep 22, 2025 •

Copy link
Copy Markdown
Collaborator

PR Type

Enhancement


Description

  • Allow WebSocket connections without Origin header for local/direct connections

  • Improve connection logging with specific messages for different connection types

  • Maintain security by still validating browser connections with Origin headers


Diagram Walkthrough

flowchart LR
  A["WebSocket Connection"] --> B["Check Origin Header"]
  B --> C["Origin is None?"]
  C -->|Yes| D["Allow Local Connection"]
  C -->|No| E["Check Allowed Origins"]
  E -->|Valid| F["Allow Browser Connection"]
  E -->|Invalid| G["Reject Connection"]
Loading

File Walkthrough

Relevant files
Enhancement
server.py
Enhanced WebSocket origin validation for local connections

agents/capture-agent-py/capture_agent/server.py

  • Modified Origin header validation logic to allow null origins
  • Added specific logging messages for different connection types
  • Maintained security for browser-based connections with Origin
    validation
+8/-3     

@netlify

netlify Bot commented Sep 22, 2025 •

Copy link
Copy Markdown

✅ Deploy Preview for sounddocsbeta ready!

Name Link
🔨 Latest commit f02e0b5
🔍 Latest deploy log https://app.netlify.com/projects/sounddocsbeta/deploys/68d17b6d5738040007f0ebe1
😎 Deploy Preview https://deploy-preview-99--sounddocsbeta.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 Security concerns

Origin validation change:
Allowing connections with no Origin header broadens the trust surface. While likely safe for non-browser clients, some reverse proxies or misconfigured clients could omit Origin. Consider adding additional checks (e.g., verify remote address is local, require auth/token, or gate behind a config flag) and ensure ALLOWED_ORIGINS cannot be bypassed via case sensitivity or whitespace.

⚡ Recommended focus areas for review

Logging Noise

Using print for connection logs may be too noisy and inconsistent with typical logging; consider using the project's logger with levels (info/warn) and include peer info for traceability.

    print("Client connected from local/direct connection (no Origin header)")
elif origin in ALLOWED_ORIGINS:
    print(f"Client connected from allowed origin: {origin}")
else:
    print(f"Connection rejected from disallowed origin: {origin}")
Origin Handling

Allowing None Origin is intentional, but consider explicitly documenting and restricting by transport (e.g., localhost or Unix socket) to avoid unintended acceptance of proxy-forwarded connections lacking Origin.

if origin is None:
    print("Client connected from local/direct connection (no Origin header)")
elif origin in ALLOWED_ORIGINS:
    print(f"Client connected from allowed origin: {origin}")
else:
    print(f"Connection rejected from disallowed origin: {origin}")

@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Handle "null" origin for local files

Modify the origin check to also allow connections when the Origin header is the
string "null".

agents/capture-agent-py/capture_agent/server.py [328-334]

-if origin is None:
-    print("Client connected from local/direct connection (no Origin header)")
+if origin is None or origin == "null":
+    print(f"Client connected from local/direct connection (Origin: {origin})")
 elif origin in ALLOWED_ORIGINS:
     print(f"Client connected from allowed origin: {origin}")
 else:
     print(f"Connection rejected from disallowed origin: {origin}")
     return
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that the Origin: "null" header, a common case for local file testing, is not handled by the new logic, and proposes a valid fix to improve support for local development.

Low
  • More

@cj-vana
cj-vana merged commit 4417ec0 into beta Sep 22, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant