Skip to content

fix: resolve open CodeQL security alerts - #9505

Merged
sriramveeraghanta merged 1 commit into
previewfrom
fix/codeql-open-alerts
Jul 29, 2026
Merged

fix: resolve open CodeQL security alerts#9505
sriramveeraghanta merged 1 commit into
previewfrom
fix/codeql-open-alerts

Conversation

@sriramveeraghanta

@sriramveeraghanta sriramveeraghanta commented Jul 29, 2026

Copy link
Copy Markdown
Member

Summary

Fixes all 4 open CodeQL code-scanning alerts:

  • apps/api/plane/authentication/adapter/oauth.py (alert feat: github importer #722, py/clear-text-logging-sensitive-data): stopped logging request headers when the user-info fetch fails — the headers carry the Bearer access token, so every OAuth failure was writing a live token to the logs.
  • apps/api/plane/authentication/adapter/base.py (alert [bug]: The initial run with docker compose failed #720, py/clear-text-logging-sensitive-data): dropped the email value from the invalid-email warning log.
  • apps/api/plane/authentication/provider/oauth/github.py (alert feat: added estimates #721, py/clear-text-logging-sensitive-data): stopped logging the organization id / user login on org-membership failure.
  • packages/editor/src/core/extensions/custom-link/extension.tsx (alert [feature]: Add Helm Chart for deploying on k8 #723, js/overly-large-range): the C0-control strip regex was written with raw 0x00/0x1F bytes, producing a range that overlaps \s. Rewritten with escaped, non-overlapping ranges covering exactly the C0 controls not already matched by \s (tab/CR/LF etc. are covered by \s itself). This also fixes the file being detected as binary data instead of UTF-8 text.

Verification

  • Regex equivalence verified exhaustively over all chars 0x00-0x2F (old vs new class, 0 diffs) and functionally against the dangerous-href cases (tab/NUL/space-prefixed javascript: hrefs still blocked; normal links unaffected).
  • oxlint (0 warnings/errors) and oxfmt --check pass on the changed file; pre-commit hooks ran clean.
  • py_compile and ruff check pass on all three Python files.

Summary by CodeRabbit

  • Security & Privacy

    • Sensitive information, including email addresses, OAuth access tokens, and account details, is no longer included in authentication warning logs.
    • Authentication error handling remains unchanged while reducing exposure of confidential data in logs.
  • Bug Fixes

    • Improved unsafe-link detection by consistently normalizing leading control characters and whitespace before checking link protocols.

- adapter/oauth.py: stop logging request headers on user-info fetch
  failure; they carry the Bearer access token (py/clear-text-logging-sensitive-data)
- adapter/base.py: drop the email value from the invalid-email warning
  log (py/clear-text-logging-sensitive-data)
- provider/oauth/github.py: stop logging organization id / user login
  on org-membership failure (py/clear-text-logging-sensitive-data)
- editor custom-link: rewrite the C0-control strip regex with escaped,
  non-overlapping ranges instead of raw control bytes overlapping \s
  (js/overly-large-range); also fixes the file being detected as binary
Copilot AI review requested due to automatic review settings July 29, 2026 14:35

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

Copy link
Copy Markdown

React Doctor found no issues. 🎉

⚠️ Warning: .github/workflows/react-doctor.yml is configured incorrectly. See below to fix.

React Doctor compares against preview to report only the issues this pull request introduces. This run couldn't complete that comparison (usually a shallow CI checkout with no merge base), so it listed every issue in the changed files, including ones that already existed on preview.

Add fetch-depth: 0 to the actions/checkout step in .github/workflows/react-doctor.yml so the checkout includes the history React Doctor needs:

 jobs:
   react-doctor:
     steps:
       - uses: actions/checkout@v5
+        with:
+          fetch-depth: 0

       - uses: millionco/react-doctor@v2

To silence this warning, set silence-missing-baseline-warning: true on the React Doctor action.

Reviewed by React Doctor for commit 4b59a01.

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 170d1adc-dc95-4b85-9a97-0faa49f239cc

📥 Commits

Reviewing files that changed from the base of the PR and between ca3b48e and 4b59a01.

📒 Files selected for processing (4)
  • apps/api/plane/authentication/adapter/base.py
  • apps/api/plane/authentication/adapter/oauth.py
  • apps/api/plane/authentication/provider/oauth/github.py
  • packages/editor/src/core/extensions/custom-link/extension.tsx

📝 Walkthrough

Walkthrough

Authentication logs now omit sensitive and identifying values. Custom-link URL normalization uses explicit escaped control-character ranges before dangerous-protocol detection.

Changes

Authentication logging updates

Layer / File(s) Summary
Authentication warning redaction
apps/api/plane/authentication/adapter/base.py, apps/api/plane/authentication/adapter/oauth.py, apps/api/plane/authentication/provider/oauth/github.py
Warnings no longer include invalid email values, OAuth authorization headers, organization identifiers, or user logins; existing exception handling remains unchanged.

Custom-link URL normalization

Layer / File(s) Summary
Explicit control-character normalization
packages/editor/src/core/extensions/custom-link/extension.tsx
isDangerousHref uses escaped C0-control and whitespace ranges when normalizing URLs before protocol checks.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: dheeru0198

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: fixing CodeQL security alerts across logging and regex issues.
Description check ✅ Passed The description is detailed and includes a summary plus verification, though the template sections aren't fully mirrored.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/codeql-open-alerts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sriramveeraghanta
sriramveeraghanta merged commit 08a7d12 into preview Jul 29, 2026
18 of 20 checks passed
@sriramveeraghanta
sriramveeraghanta deleted the fix/codeql-open-alerts branch July 29, 2026 14:58
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.

3 participants