forked from makeplane/plane
-
Notifications
You must be signed in to change notification settings - Fork 0
chore(ci): add fork-side SSO audit script + workflow #32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+368
−1
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
139daf4
chore(ci): add fork-side SSO audit script + workflow
awais786 4fb4805
chore(ci): address four Copilot review comments on the fork audit
awais786 ece9f69
chore(ci): use portable word boundary in row 20 call detection
awais786 ce67aa6
chore(ci): tighten Row 14 success message — clarify what is/isn't ver…
awais786 19d580a
Potential fix for pull request finding
awais786 8a1b118
chore(ci): split sso audit comment into least-privilege job
Copilot 8ae1883
chore(ci): harden audit output rendering and setup python
Copilot f9cfd4a
chore(ci): use html-entity escaping for markdown table pipes
Copilot 7b36291
chore(ci): document markdown table cell escaping helper
Copilot ea7d9ef
chore(ci): quote markdown escape helper input
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| name: SSO fork audit | ||
|
|
||
| # Runs scripts/sso-audit.sh against this fork's auth code to verify | ||
| # satisfaction of the cross-app SSO contract at | ||
| # awais786/sso-rules-moneta:openspec/specs/proxy-auth-middleware/spec.md. | ||
| # | ||
| # Covers the fork-side rows that foss-server-bundle-devstack's CI can't | ||
| # check (no fork code on disk in the bundle): today rows 14, 20, 21 from | ||
| # SKILL.md §5. Exits 1 on security-critical violations (row 20 — session- | ||
| # identity reconciliation) so the merge gate blocks the stale-session | ||
| # leak class of bug. | ||
| # | ||
| # When upstream sso-rules-moneta adds a new fork-side row, vendor the | ||
| # updated check into scripts/sso-audit.sh and re-run this workflow. | ||
|
|
||
| on: | ||
| pull_request: | ||
| paths: | ||
| - 'apps/api/plane/authentication/**' | ||
| - 'apps/web/core/store/user/**' | ||
| - 'apps/web/core/services/auth.service.ts' | ||
| - 'apps/web/core/lib/wrappers/authentication-wrapper.tsx' | ||
| - 'scripts/sso-audit.sh' | ||
| - '.github/workflows/sso-audit.yml' | ||
| push: | ||
| branches: [foss-main] | ||
| schedule: | ||
| # 09:00 UTC every Monday — pre-week sanity check | ||
| - cron: '0 9 * * 1' | ||
| workflow_dispatch: | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| jobs: | ||
| sso-fork-audit: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: '3.12' | ||
|
|
||
| - name: Run fork audit | ||
| id: audit | ||
| run: | | ||
| set -o pipefail | ||
| if bash scripts/sso-audit.sh | tee audit-output.md; then | ||
| echo "audit_exit=0" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "audit_exit=$?" >> "$GITHUB_OUTPUT" | ||
| fi | ||
|
|
||
|
Comment on lines
+38
to
+56
|
||
| - name: Publish to job summary | ||
| if: always() | ||
| run: cat audit-output.md >> "$GITHUB_STEP_SUMMARY" | ||
|
|
||
| - name: Upload audit report | ||
| if: always() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: sso-fork-audit-report | ||
| path: audit-output.md | ||
|
|
||
| - name: Fail on security-critical violations | ||
| if: steps.audit.outputs.audit_exit != '0' | ||
| run: | | ||
| echo "::error::Security-critical SSO contract violation in fork audit. See table for the failing row and fix." | ||
| exit 1 | ||
|
|
||
| sso-fork-audit-comment: | ||
| needs: sso-fork-audit | ||
| if: | | ||
| always() && | ||
| needs.sso-fork-audit.result != 'cancelled' && | ||
| github.event_name == 'pull_request' && | ||
| github.event.pull_request.head.repo.full_name == github.repository | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
| steps: | ||
| - name: Download audit report | ||
| uses: actions/download-artifact@v4 | ||
| with: | ||
| name: sso-fork-audit-report | ||
|
|
||
| - name: Post sticky PR comment | ||
| continue-on-error: true | ||
| uses: marocchino/sticky-pull-request-comment@v2 | ||
| with: | ||
| header: sso-fork-audit | ||
| path: audit-output.md | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,269 @@ | ||
| #!/usr/bin/env bash | ||
| # | ||
| # sso-audit.sh — Plane-side fork audit against the cross-app SSO contract. | ||
| # | ||
| # ============================================================================ | ||
| # Covers the fork-side rows of awais786/sso-rules-moneta:openspec/specs/ | ||
| # proxy-auth-middleware/spec.md that a deterministic bash check can verify | ||
| # against Plane's source tree. Catches regressions BEFORE they reach | ||
| # foss-server-bundle-devstack, where the same rows currently emit `?` (no | ||
| # fork code on disk in the bundle CI). | ||
| # | ||
| # Together with the bundle-side audit (foss-server-bundle-devstack/scripts/ | ||
| # audit-sso.sh), the contract is now ~14 of 21 rows deterministic without | ||
| # any LLM invocation. The remaining rows (identity-managed UI gating, | ||
| # display-name UUID check) need either runtime SSO state or AST-level | ||
| # semantic analysis — those defer to the LLM-backed | ||
| # /sso-rules:audit-all-apps slash command. | ||
| # | ||
| # Exit codes: | ||
| # 0 — no SECURITY-CRITICAL rows failed. Non-security rows may still be | ||
| # reported as ❌; those findings are informational/non-gating. | ||
| # 1 — at least one SECURITY-CRITICAL row failed (today: row 20 session- | ||
| # identity reconciliation). These violations re-open the cross-user | ||
| # identity-leak class of bug. | ||
| # | ||
| # Rows covered: | ||
| # Row 14 — logout shape: SPA logout file MUST NOT call /oauth2/sign_out. | ||
| # (The portal-host redirect target and the no-POST-/auth/sign-out/ | ||
| # properties from logout-flow spec are NOT checked here — they | ||
| # need either runtime context or AST-level analysis. The bash | ||
| # audit covers only the literal /oauth2/sign_out grep, which is | ||
| # the most common regression vector.) | ||
| # Row 20 — session-identity reconciliation (Rule 2 mismatch flush): | ||
| # proxy_auth.py MUST call django.contrib.auth.logout(request) | ||
| # on identity mismatch. Without this, the stale-session-on-user- | ||
| # switch leak returns. SECURITY-CRITICAL. | ||
| # Row 21 — email-shape detection MUST NOT use polynomial-backtracking | ||
| # regex. Plane uses substring check today, so this row is | ||
| # informational — the audit fires only if a regex is reintroduced. | ||
| # | ||
| # Source of truth: awais786/sso-rules-moneta:openspec/specs/proxy-auth- | ||
| # middleware/spec.md. When upstream adds a new fork-side row, vendor the | ||
| # updated check here. | ||
| # ============================================================================ | ||
|
|
||
| set -euo pipefail | ||
|
|
||
| PROXY_AUTH="apps/api/plane/authentication/middleware/proxy_auth.py" | ||
| PROXY_AUTH_UTILS="apps/api/plane/authentication/middleware/proxy_auth_utils.py" | ||
| LOGOUT_SPA="apps/web/core/store/user/index.ts" | ||
|
|
||
| # Output state — populated by each check_row_N function, printed at the end. | ||
| declare -a ROW_STATUS=() | ||
| declare -a ROW_TITLES=( | ||
| "logout shape: SPA logout does not call /oauth2/sign_out" | ||
| "session-identity reconciliation present (Rule 2 mismatch flush)" | ||
| "email-shape detection uses substring/indexOf, not polynomial regex" | ||
| ) | ||
| declare -a ROW_NOTES=() | ||
|
|
||
| # Row numbers per the cross-app SKILL.md §5 table. We only emit a subset | ||
| # here; bundle CI emits 1-21. | ||
| declare -a ROW_NUMBERS=(14 20 21) | ||
|
|
||
| # Security-critical row indices (0-indexed into ROW_NUMBERS above). | ||
| SECURITY_CRITICAL=(1) # index 1 → row 20 | ||
|
|
||
| SECURITY_CRITICAL_FAILS=0 | ||
|
|
||
| record() { | ||
| local idx=$1 status=$2 note=$3 | ||
| ROW_STATUS[$idx]="$status" | ||
| ROW_NOTES[$idx]="$note" | ||
| if [[ "$status" == "❌" ]]; then | ||
| for c in "${SECURITY_CRITICAL[@]}"; do | ||
| if [[ "$c" -eq "$idx" ]]; then | ||
| SECURITY_CRITICAL_FAILS=$((SECURITY_CRITICAL_FAILS + 1)) | ||
| return | ||
| fi | ||
| done | ||
| fi | ||
| } | ||
|
|
||
| escape_markdown_cell() { | ||
| # Escape table-sensitive characters so notes render as a single markdown cell. | ||
| # Input: arbitrary note text. Output: `|` as HTML entity + newlines as <br>. | ||
| local cell="$1" | ||
| cell=${cell//|/|} | ||
| cell=${cell//$'\n'/'<br>'} | ||
| printf '%s' "$cell" | ||
| } | ||
|
|
||
| # ============================================================================ | ||
| # Row 14 (idx 0): logout shape — narrow check | ||
| # | ||
| # SPA logout MUST NOT contain `/oauth2/sign_out` literal — the bundle's | ||
| # logout model is navigation-only at the per-app layer. The portal handles | ||
| # oauth2-proxy clearing. This check enforces only that property; the other | ||
| # properties from logout-flow spec (portal-host redirect target, no POST | ||
| # /auth/sign-out/) are not verified here. | ||
| # ============================================================================ | ||
| check_row_14() { | ||
| if [[ ! -f "$LOGOUT_SPA" ]]; then | ||
| record 0 "?" "$LOGOUT_SPA not found — skipping" | ||
| return | ||
| fi | ||
|
|
||
| if grep -qE '/oauth2/sign_?out' "$LOGOUT_SPA"; then | ||
| local line | ||
| line=$(grep -nE '/oauth2/sign_?out' "$LOGOUT_SPA" | head -1) | ||
| record 0 "❌" "Logout calls \`/oauth2/sign_out\` at $LOGOUT_SPA:$line — per logout-flow spec, per-app Logout is navigation-only. Drop the call; portal 'Logout all' handles oauth2-proxy clearing." | ||
|
Comment on lines
+109
to
+111
|
||
| return | ||
| fi | ||
|
|
||
| record 0 "✅" "$LOGOUT_SPA does not invoke \`/oauth2/sign_out\` (this row verifies only that the SPA doesn't try to clear the upstream proxy cookie itself; that's the portal's job. The SPA MAY still POST \`/auth/sign-out/\` — tolerated drift, see logout-flow spec)" | ||
| } | ||
|
|
||
| # ============================================================================ | ||
| # Row 20 (idx 1): session-identity reconciliation | ||
| # | ||
| # proxy_auth.py MUST call django.contrib.auth.logout (imported as `logout` or | ||
| # under any local alias) on identity mismatch. The presence of the import + | ||
| # call is the deterministic signal; the bash audit can't verify the call | ||
| # site's *position* relative to the mismatch detection, only that it exists. | ||
| # That's a spec-conformance approximation; the test suite at | ||
| # apps/api/plane/authentication/tests/test_proxy_auth.py pins the exact | ||
| # behaviour. | ||
| # | ||
| # The detection is resilient to: | ||
| # - whitespace around the argument: `logout(request)`, `logout( request )`, | ||
| # `logout(\nrequest\n)` | ||
| # - keyword form: `logout(request=request)` | ||
| # - parenthesized multiline imports: | ||
| # from django.contrib.auth import ( | ||
| # login, | ||
| # logout, | ||
| # ) | ||
| # - aliased imports: `from django.contrib.auth import logout as django_logout` | ||
| # - mixed imports on one line: `from django.contrib.auth import login, logout` | ||
| # | ||
| # SECURITY-CRITICAL: without this call, the stale-session leak returns. | ||
| # ============================================================================ | ||
| check_row_20() { | ||
| if [[ ! -f "$PROXY_AUTH" ]]; then | ||
| record 1 "?" "$PROXY_AUTH not found — skipping" | ||
| return | ||
| fi | ||
|
|
||
| # Resolve the in-scope name for django.contrib.auth.logout. | ||
| # If the file imports it under an alias, use that. Otherwise default to | ||
| # `logout`. Use python -c so we correctly handle multiline parenthesized | ||
| # imports without trying to write a multi-line regex in bash. | ||
| local logout_name | ||
| logout_name=$(python3 - "$PROXY_AUTH" <<'PY' 2>/dev/null || true | ||
| import ast, sys | ||
| src = open(sys.argv[1]).read() | ||
| try: | ||
| tree = ast.parse(src) | ||
| except SyntaxError: | ||
| sys.exit(0) | ||
| for node in ast.walk(tree): | ||
| if isinstance(node, ast.ImportFrom) and node.module == "django.contrib.auth": | ||
| for alias in node.names: | ||
| if alias.name == "logout": | ||
| print(alias.asname or "logout") | ||
| sys.exit(0) | ||
| PY | ||
| ) | ||
|
|
||
| if [[ -z "$logout_name" ]]; then | ||
| record 1 "❌" "$PROXY_AUTH does NOT import django.contrib.auth.logout. The cross-app spec (proxy-auth-middleware Rule 2) requires the middleware to call logout(request) when the proxy header asserts a different identity than the existing Django session. Without it, the stale-session-on-user-switch leak returns. Fix: add \`from django.contrib.auth import logout\` AND invoke \`logout(request)\` immediately on mismatch, before any bail-out path." | ||
| return | ||
| fi | ||
|
|
||
| # Look for a call to <logout_name>(...) anywhere in the file. The argument | ||
| # can include whitespace, line breaks, or `request=request` keyword form; | ||
| # the audit only cares that the call exists, not its exact shape. | ||
| # | ||
| # Portable word boundary at the start: `(^|[^[:alnum:]_])` matches either | ||
| # start-of-line or any non-word character before the alias name. Using | ||
| # POSIX character classes rather than `\b` because `\b` is a GNU extension | ||
| # to `grep -E` and is undefined on BSD/POSIX strict implementations | ||
| # (`grep` on macOS, busybox, Alpine, etc. may interpret it as literal | ||
| # backspace or just `b`). The right side doesn't need a boundary because | ||
| # `[[:space:]]*\(` already requires the next non-space char to be `(`. | ||
| local call_pattern="(^|[^[:alnum:]_])${logout_name}[[:space:]]*\\(" | ||
| if grep -qE "$call_pattern" "$PROXY_AUTH"; then | ||
|
Comment on lines
+175
to
+187
|
||
| record 1 "✅" "django.contrib.auth.logout imported (as \`$logout_name\`) and invoked at least once in $PROXY_AUTH — Rule 2 mismatch flush in place" | ||
| return | ||
| fi | ||
|
|
||
| record 1 "❌" "$PROXY_AUTH imports django.contrib.auth.logout (as \`$logout_name\`) but never calls it. Fix: invoke \`$logout_name(request)\` on identity mismatch before falling through to the unauthenticated path." | ||
| } | ||
|
|
||
| # ============================================================================ | ||
| # Row 21 (idx 2): polynomial-regex avoidance in email-shape detection | ||
| # | ||
| # Plane uses substring check (`"@" in email`) today, so this is a regression | ||
| # guard. The audit fires only if a Python regex matching the unanchored | ||
| # `[^\s@]+@[^\s@]+\.[^\s@]+` pattern is introduced. CodeQL's | ||
| # `py/polynomial-redos` (or `js/polynomial-redos`) flags this on adversarial | ||
| # input. | ||
| # ============================================================================ | ||
| check_row_21() { | ||
| local files=() | ||
| [[ -f "$PROXY_AUTH" ]] && files+=("$PROXY_AUTH") | ||
| [[ -f "$PROXY_AUTH_UTILS" ]] && files+=("$PROXY_AUTH_UTILS") | ||
|
|
||
| if [[ ${#files[@]} -eq 0 ]]; then | ||
| record 2 "?" "No proxy_auth files found — skipping" | ||
| return | ||
| fi | ||
|
|
||
| # Look for the canonical polynomial-backtracking shape: | ||
| # [^\s@]+@[^\s@]+\.[^\s@]+ | ||
| # in any of the inspected files. Allow trivial whitespace variations. | ||
| local hits | ||
| hits=$(grep -nE '\[\^[\\\\]s@\]\+@\[\^[\\\\]s@\]\+\\\.\[\^[\\\\]s@\]\+' "${files[@]}" 2>/dev/null || true) | ||
|
|
||
| if [[ -n "$hits" ]]; then | ||
| record 2 "❌" "Polynomial-backtracking email-shape regex detected: $hits. Rewrite to indexOf/substring-based check per openspec proxy-auth-middleware §'email-shape detection SHALL avoid polynomial-backtracking regex'. Reference: Outline's normalizeProxyEmail in Pressingly/outline#19." | ||
| return | ||
| fi | ||
|
|
||
| record 2 "✅" "No polynomial-backtracking email-shape regex in ${files[*]}; using substring check or other O(n) detection" | ||
| } | ||
|
|
||
| # ============================================================================ | ||
| # Run checks | ||
| # ============================================================================ | ||
| check_row_14 | ||
| check_row_20 | ||
| check_row_21 | ||
|
|
||
| # ============================================================================ | ||
| # Print table | ||
| # ============================================================================ | ||
| echo "## Plane SSO Fork Audit" | ||
| echo | ||
| echo "Cross-app contract: https://github.com/awais786/sso-rules-moneta/blob/main/openspec/specs/proxy-auth-middleware/spec.md" | ||
| echo "Row numbers match the 21-row table at https://github.com/awais786/sso-rules-moneta/blob/main/skills/app-rules/SKILL.md#5-report" | ||
| echo | ||
| echo "| Row | Invariant | Status | Notes |" | ||
| echo "|-----|-----------|--------|-------|" | ||
| for i in "${!ROW_TITLES[@]}"; do | ||
| note="$(escape_markdown_cell "${ROW_NOTES[$i]:-}")" | ||
| printf "| %d | %s | %s | %s |\n" \ | ||
| "${ROW_NUMBERS[$i]}" "${ROW_TITLES[$i]}" "${ROW_STATUS[$i]:-?}" "$note" | ||
| done | ||
| echo | ||
|
|
||
| # ============================================================================ | ||
| # Summary + exit code | ||
| # ============================================================================ | ||
| TOTAL_FAILS=0 | ||
| for s in "${ROW_STATUS[@]}"; do | ||
| [[ "$s" == "❌" ]] && TOTAL_FAILS=$((TOTAL_FAILS + 1)) | ||
| done | ||
|
|
||
| if [[ "$TOTAL_FAILS" -eq 0 ]]; then | ||
| echo "**All fork-side invariants hold.**" | ||
| exit 0 | ||
| fi | ||
|
|
||
| echo "**$TOTAL_FAILS violations.** Security-critical (row 20): $SECURITY_CRITICAL_FAILS." | ||
| if [[ "$SECURITY_CRITICAL_FAILS" -gt 0 ]]; then | ||
| exit 1 | ||
| fi | ||
| exit 0 | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented in
8a1b118. I split the workflow into two jobs for least privilege: the audit job now runs withcontents: readonly, and a separate PR-only same-repo comment job haspull-requests: writeand posts the sticky comment from an uploaded/downloaded audit artifact. Security-critical audit gating behavior is preserved.