Skip to content

feat: add simple web server with query handling - #33

Closed
ralphbean wants to merge 1 commit into
mainfrom
test/review-labels-2196
Closed

ralphbean wants to merge 1 commit into
mainfrom
test/review-labels-2196

Conversation

@ralphbean

Copy link
Copy Markdown
Contributor

Summary

Adds a basic Python HTTP server that handles GET and POST requests.

Test plan

  • Manual testing

Signed-off-by: Ralph Bean <rbean@redhat.com>
@ralphbean

ralphbean commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Review

Findings

High

  • [error-handling] app.py:18 — The Content-Length header value is passed directly to int() without error handling. A malformed header (e.g., Content-Length: abc) crashes the handler with ValueError. No upper bound is enforced on body size, risking memory exhaustion with large values.
    Remediation: Wrap int() in try/except, return 400 on ValueError. Add a MAX_BODY size check and return 413 if exceeded.

  • [injection] app.py:9 — Reflected Cross-Site Scripting (XSS). User input from self.path.split('?q=')[-1] is interpolated directly into an HTML response (Content-Type: text/html) without escaping. The code's own comment acknowledges: "Bug: unsanitized user input reflected in response."
    Remediation: Use html.escape(user_input) before embedding in HTML, or set Content-Type: text/plain.

  • [architectural-misalignment] app.py — The repository is a Konflux testing component with a bash entrypoint (entrypoint.sh: echo hello world) and a UBI8-based Dockerfile that doesn't install Python. Adding a Python HTTP server is architecturally misaligned — the Dockerfile doesn't reference app.py, so this code cannot run in the defined container.
    Remediation: Update Dockerfile to install Python and wire app.py as the entrypoint, or remove if it doesn't align with the component's purpose.

Medium

  • [missing-authorization] app.py — Non-trivial change (30 new lines, new component) with no linked issue. AGENTS.md principle Connect to fullsend agent pipeline #3 states every changed line must trace to issue scope.

  • [scope-creep] app.py:16 — PR title says "web server with query handling" but the implementation also includes POST request handling with body echoing, which is not mentioned in the title.

  • [missing-documentation] README.md — PR introduces a Python HTTP server with GET/POST endpoints and a PORT environment variable, but README.md doesn't document any of this.

  • [logic-error] app.py:9 — Query parsing self.path.split('?q=')[-1] fails in multiple edge cases: no ?q= parameter yields the raw path, values containing ?q= are truncated, and URL-encoded characters are never decoded.

  • [injection] app.py:23 — POST body is echoed verbatim without setting a Content-Type header. Browsers may sniff the response as HTML, enabling reflected XSS via cross-origin form submission.

  • [test-adequacy] app.py:1 — New web server with two handlers and zero automated tests. No coverage for query parsing, POST handling, error paths, or startup logic.

Low

  • [naming-misalignment] app.py — Generic filename doesn't convey purpose within the Konflux testing context.

  • [data-exposure] app.py:28 — Server binds to all interfaces (0.0.0.0) with no authentication.

  • [documentation-format] app.py:10 — Comment "Bug: unsanitized user input reflected in response" is ambiguous about whether the vulnerability is intentional for testing.

Info

  • [naming-conventions] app.py:6 — Class name Handler is generic; TestRequestHandler would be more descriptive.

  • [code-organization] app.py:1 — Python app placed in repo root alongside infrastructure files without organizational structure.


Labels: PR adds a new feature (web server) with known security and correctness bugs in the implementation.

Previous run

Review

Findings

High

  • [xss] app.py:14 — User input from self.path is interpolated unsanitized into an HTML response served as text/html. An attacker can craft a URL such as /?q=<script>alert(1)</script> to execute arbitrary JavaScript in a victim's browser. The code itself acknowledges this with the comment # Bug: unsanitized user input reflected in response.
    Remediation: HTML-escape user_input using html.escape(user_input, quote=True) before embedding in the response.

  • [architectural-incoherence] app.py — The new Python web server is architecturally disconnected from the existing repository. The Dockerfile copies only entrypoint.sh and sets it as ENTRYPOINT; integration tests validate "hello world" output from that script. app.py introduces a completely different runtime that is not referenced by the build pipeline, Dockerfile, or any existing component.
    Remediation: Either modify the Dockerfile and entrypoint.sh to run app.py, or explain the architectural rationale for the addition.

Medium

  • [missing-authorization] app.py — This PR adds a new web server component (30 lines of new production code) but has no linked issue authorizing the work. Non-trivial changes require explicit authorization.

  • [scope-mismatch] app.py — The PR is labeled feat: but the repository's stated purpose is testing Konflux infrastructure. Adding an HTTP server appears to be scope creep unless there is a documented requirement to test HTTP workloads.

  • [logic-error] app.py:9 — Query parameter extraction via self.path.split("?q=")[-1] is fragile: (1) no query string returns the entire path as "results", (2) multiple ?q= substrings use only the last segment, (3) additional query parameters like &page=2 are included in the result. Use urllib.parse.urlparse and parse_qs instead.

  • [error-handling] app.py:18int(self.headers.get("Content-Length", 0)) raises ValueError if the header is present but non-integer. While BaseHTTPRequestHandler catches the exception, the client receives a raw 500 instead of a proper 400 response.

  • [missing-test] app.py:1 — New web server with request handling logic but zero automated tests. The test plan is "Manual testing" only. Edge cases in query parsing and POST body handling would be caught by basic unit tests.

  • [injection-vuln] app.py:23do_POST echoes the raw request body without setting a Content-Type header. Browsers may MIME-sniff the response; if interpreted as HTML, this becomes a reflected XSS vector. Set an explicit safe Content-Type and add X-Content-Type-Options: nosniff.

  • [missing-doc] README.md — The PR introduces a new web server with a PORT environment variable and GET/POST endpoints, but README.md has no mention of how to run, configure, or use it.

Low

  • [dead-code-introduction] app.py — The file is completely unused by the existing build/deployment pipeline. See also: [architectural-incoherence] finding above.

  • [missing-doc] Dockerfile:5 — Dockerfile uses entrypoint.sh as entrypoint but does not reference app.py. See also: [architectural-incoherence] finding above.

  • [error-handling] app.py:17 — Docstring says "Handle POST with JSON body" but no JSON parsing occurs. The handler reads raw bytes and echoes them back, which may mislead callers.

  • [network-exposure] app.py:28 — Server binds to all network interfaces ("" = 0.0.0.0), maximizing attack surface when combined with the XSS vulnerabilities above.

Info

  • [trajectory-divergence] app.py — Repository history shows only infrastructure/workflow changes. Adding application logic represents a significant divergence from the established project trajectory.

  • [missing-doc] README.md — No Python runtime requirements documented. While only stdlib modules are used, the Python version requirement is unstated.


Labels: PR adds a new feature (web server) and needs more information (no linked issue, architectural justification missing)

@ralphbean

Copy link
Copy Markdown
Contributor Author

Test PR for fullsend-ai/fullsend#2196 — manual validation complete.

@ralphbean ralphbean closed this Jun 12, 2026
@ralphbean
ralphbean deleted the test/review-labels-2196 branch June 12, 2026 01:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant