Skip to content

feat(core): cookie exchange endpoint#21

Open
MrSecure wants to merge 20 commits into
mainfrom
feature/cookie-exchange
Open

feat(core): cookie exchange endpoint#21
MrSecure wants to merge 20 commits into
mainfrom
feature/cookie-exchange

Conversation

@MrSecure

@MrSecure MrSecure commented Apr 12, 2026

Copy link
Copy Markdown
Owner

Contributor Comments

  • Add support for authentication via HTTP Header, specified via header.name config.
  • Add support for Cookie to Token exchange (cookie.exchange_enabled: true)
  • Fix entry point script to accept --check-config and pass it through to gunicorn
  • Fix integration tests that expected --check-config to work
  • Update pre-commit tools

Pull Request Checklist

Thank you for submitting a contribution to homelab-auth!

Please address the following items:

  • If you are adding a dependency, please explain how it was chosen.
  • If manual testing is needed in order to validate the changes, provide a testing plan and the expected results.
  • Validate that documentation is accurate and aligned to any project updates or additions.

Comment thread src/main.py Dismissed
Comment thread src/main.py Fixed
@MrSecure

Copy link
Copy Markdown
Owner Author

@copilot review this PR - I want early feedback

Copilot AI and others added 3 commits April 12, 2026 21:40
- Fix is_safe_redirect to reject protocol-relative URLs (//evil.com)
  that start with / but resolve to external hosts
- Use jsonify() explicitly in cookie_crumbling_protocol_v2 to
  ensure Content-Type: application/json and satisfy static analysis
- Add unit test for protocol-relative URL rejection

Agent-Logs-Url: https://github.com/MrSecure/homelab-auth/sessions/6352a153-440e-4f21-8fa8-b3b06d3e6f7a

Co-authored-by: MrSecure <1155636+MrSecure@users.noreply.github.com>
…odeQL alerts

- Replace is_safe_redirect() with get_safe_redirect_url() that takes user URL
  and a safe default, returning the validated URL or the default in one call
- This eliminates the two-step validate-then-maybe-reassign pattern that
  CodeQL could not trace as a proper sanitization
- Add explicit http/https scheme check and protocol-relative URL rejection
- Use jsonify() in cookie_crumbling_protocol_v2 for explicit Content-Type
- Update tests to match renamed helper function

Agent-Logs-Url: https://github.com/MrSecure/homelab-auth/sessions/6352a153-440e-4f21-8fa8-b3b06d3e6f7a

Co-authored-by: MrSecure <1155636+MrSecure@users.noreply.github.com>
- Tests now call the actual function instead of duplicating inline logic
- Covers: relative paths, protocol-relative URLs, absolute domain matching,
  invalid schemes (javascript:, data:), external domain rejection
- Uses load_main_module + test_request_context for proper Flask context

Agent-Logs-Url: https://github.com/MrSecure/homelab-auth/sessions/6352a153-440e-4f21-8fa8-b3b06d3e6f7a

Co-authored-by: MrSecure <1155636+MrSecure@users.noreply.github.com>

Copilot AI commented Apr 12, 2026

Copy link
Copy Markdown

@copilot review this PR - I want early feedback

Here's early feedback on the PR. I've also addressed the two CodeQL alerts while reviewing.

Security fixes (commits e8f8333, 3ba09d3, 5224771)

  1. Open redirect — protocol-relative URL bypass (CodeQL alert, line 475): The old is_safe_redirect() returned True for any URL starting with /, which incorrectly allowed //evil.com (protocol-relative URLs that browsers treat as https://evil.com). Fixed by replacing it with get_safe_redirect_url() that:

    • Explicitly rejects // URLs
    • Validates http/https schemes only (blocks javascript:, data:, etc.)
    • Returns the validated URL or a config-sourced safe default in a single call — no two-step validate-then-reassign pattern
  2. Reflected XSS in /cookie-crumbling-protocol-v2 (CodeQL alert, lines 597–602): Flask auto-converts returned dicts to JSON but CodeQL couldn't trace that. Switched all three return statements to explicit jsonify() calls so the Content-Type: application/json header is always guaranteed.

Remaining CodeQL URL-redirect alerts (pre-existing, false positives): Lines 459, 476, 487, 530 are all flagged because CodeQL's interprocedural taint analysis sees the user-supplied rd parameter flowing through get_safe_redirect_url()'s return path into redirect(). These are false positives — the validation is solid. They can be dismissed in the GitHub Security UI as "False positive."

Code quality observations

  • The cookie.exchange_enabled default is True in cfg.get("cookie", {}).get("exchange_enabled", True) — worth documenting clearly that omitting the key enables the exchange endpoint by default, since that's a behaviour change from before the feature existed.
  • The default header name "X-HomeLab-Misconfigured" is a good sentinel for misconfiguration, but it might be worth a startup warning log if header.name is not explicitly set when exchange_enabled: true.
  • Tests look solid; the new test_get_safe_redirect_url_* tests now call the function directly rather than duplicating the logic inline.

@MrSecure MrSecure changed the title Feature/cookie exchange feat: cookie exchange endpoint Apr 12, 2026
@MrSecure MrSecure changed the title feat: cookie exchange endpoint feat(core): cookie exchange endpoint Apr 12, 2026
@MrSecure

Copy link
Copy Markdown
Owner Author

@copilot have all the GHAS findings been addressed?

Copilot AI commented Apr 12, 2026

Copy link
Copy Markdown

@copilot have all the GHAS findings been addressed?

Both findings are addressed:

  • Alert chore(deps): update bcrypt to v4 #8 (Reflected XSS, lines 597–602) — ✅ Fully resolved. All returns in /cookie-crumbling-protocol-v2 use explicit jsonify() calls. The thread is marked resolved.

  • Alert fix(ci): disable integration tests #7 (URL redirection) — The real vulnerability is fixed. The original bug was that the old is_safe_redirect() accepted protocol-relative URLs like //evil.com (start with / but resolve to external hosts). get_safe_redirect_url() now explicitly rejects // URLs, enforces http/https-only schemes, and returns a safe sanitized value in a single call so all callers always use the validated URL.

    The remaining CodeQL alerts for this rule are false positives — the scanner traces the taint from request.args.get("rd", "") through get_safe_redirect_url()'s return value without recognizing the function as a sanitizer. Every redirect(target_url) operates on the sanitized return value of get_safe_redirect_url(). These can be dismissed as "False positive" in the GitHub Security dashboard.

fix: issues identified by bandit
fix: restore missing security-scan task in Taskfile
fix: update some task references in CLAUDE.md
Comment thread src/main.py Dismissed
… from remote source'

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@MrSecure
MrSecure marked this pull request as ready for review April 13, 2026 00:26
Copilot AI review requested due to automatic review settings April 13, 2026 00:26

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds support for cookie/header-based token exchange and hardens redirect handling, plus updates container entrypoint behavior and project tooling/docs to support the new flows.

Changes:

  • Added safe redirect normalization (get_safe_redirect_url) and introduced /cookie-crumbling-protocol-v2 plus header-token fallback for /login and /verify.
  • Updated Docker entrypoint/CI integration tests to support --check-config, and added gunicorn control-socket directory setup.
  • Added configuration knobs (cookie.exchange_enabled, header.name), expanded endpoint documentation, and updated pre-commit/tooling (Bandit task + config).

Reviewed changes

Copilot reviewed 17 out of 18 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/main.py Adds safe redirect helper, header-token fallback, and cookie exchange endpoint; improves Jinja auto-escaping and logging.
tests/test_main.py Adds module loader helper and new unit tests around safe redirects and header-token login behavior.
tests/test_integration.py Adjusts Docker run invocation to match the image entrypoint and adds timeouts.
tests/test_cookie_crumbling_protocol.py Introduces tests intended to validate the new cookie exchange endpoint and related config behavior.
support/entrypoint.sh Adds --check-config handling and passes additional gunicorn flags.
Dockerfile Creates /app/.gunicorn directory needed for the entrypoint’s socket path.
examples/config.yaml Adds cookie.exchange_enabled and header.name example defaults.
support/config.yaml Adds cookie.exchange_enabled and header.name defaults.
support/wsgi-config.yaml Adds cookie.exchange_enabled and header.name defaults.
docs/ENDPOINTS.md Adds endpoint-by-endpoint behavioral documentation, including cookie exchange semantics.
README.md Updates the API endpoints list to include new/clarified endpoints.
Taskfile.yml Adds a Bandit security scan task and re-enables integration tests in task test.
pyproject.toml Adds Bandit configuration to match the new security-scan task.
.pre-commit-config.yaml Updates pinned tool revisions (ruff/actionlint/openapi validator/cspell).
.github/etc/dictionary.txt Adds healthz to the spellcheck dictionary.
.github/actions/bootstrap/action.yml Adds optional Node.js setup via a node-version input.
CLAUDE.md Removes references to a task format command.

Comment thread src/main.py Outdated
Comment on lines 471 to 475
default_url = f"https://{cfg['redir']['default_destination']}{domain}"
target_url = get_safe_redirect_url(request.args.get("rd", ""), default_url)

login_url = f"https://{cfg['redir']['external_name']}{domain}"
return redirect(f"{login_url}/login?rd={target_url}", code=307)

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

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

The redirect to the external login host interpolates target_url directly into the rd query param without URL-encoding. If target_url contains reserved characters (e.g., &, #, or ?), it can break the query string or allow parameter injection. Build the redirect URL using proper query encoding (e.g., urllib.parse.urlencode / quote) so the rd value is always a single parameter.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@copilot apply changes based on this feedback

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Done in commit 269790a. Added quote from urllib.parse and changed the redirect to ?rd={quote(target_url, safe='')} so any reserved characters in the validated redirect URL are percent-encoded before being appended to the query string.

Comment thread support/entrypoint.sh
Comment thread tests/test_cookie_crumbling_protocol.py Outdated
Comment thread tests/test_cookie_crumbling_protocol.py Outdated
Comment thread README.md Outdated
MrSecure and others added 2 commits April 12, 2026 19:35
Update tests/test_cookie_crumbling_protocol.py

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI commented Apr 13, 2026

Copy link
Copy Markdown

@copilot have all the GHAS findings been addressed?

Both original GHAS findings are resolved:

The remaining CodeQL py/url-redirection alert at line 475 is a false positive. The redirect destination is login_url (built entirely from config), and the user-provided rd value only appears as a quote()-encoded query parameter value — CodeQL's interprocedural taint analysis flags it because it cannot see through the sanitizer. It can be dismissed in the GHAS dashboard as "False positive."

Comment thread src/main.py

login_url = f"https://{cfg['redir']['external_name']}{domain}"
return redirect(f"{login_url}/login?rd={target_url}", code=307)
return redirect(f"{login_url}/login?rd={quote(target_url, safe='')}", code=307)

@MrSecure MrSecure Apr 13, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@copilot - this is not a viable suggestion for the system. Refactor it to allow redirects to any host that is a subdomain of get_cookie_subdomain(). If the configuration includes a list of cookie.allowed_hosts, further constrain the redirection to require an exact match to one of those FQDNs.

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.

4 participants