-
Notifications
You must be signed in to change notification settings - Fork 0
chore(ci): add fork-side SSO audit script + workflow #20
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
Changes from all commits
a71e693
9596c2b
c2b3e7c
6b26a1c
c7ce838
fc31ab9
ff9aeff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| 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 fork-side rows 14, 20, 21 from SKILL.md §5. Exits 1 on security- | ||
| # critical violations (row 20 — session-identity reconciliation). | ||
|
|
||
| on: | ||
| pull_request: | ||
| paths: | ||
| - 'server/middlewares/authentication.ts' | ||
| - 'server/middlewares/authentication.test.ts' | ||
| - 'app/stores/AuthStore.ts' | ||
| - 'scripts/sso-audit.sh' | ||
| - '.github/workflows/sso-audit.yml' | ||
| push: | ||
| branches: [foss-main] | ||
| schedule: | ||
| - cron: '0 9 * * 1' | ||
| workflow_dispatch: | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| jobs: | ||
| sso-fork-audit: | ||
| permissions: | ||
| contents: read | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v5 | ||
|
|
||
| - 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 | ||
|
|
||
| - name: Publish to job summary | ||
| if: always() | ||
| run: cat audit-output.md >> "$GITHUB_STEP_SUMMARY" | ||
|
|
||
| - name: Upload audit output | ||
| if: always() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: sso-audit-output | ||
| 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() && | ||
| 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 output | ||
| uses: actions/download-artifact@v4 | ||
| with: | ||
| name: sso-audit-output | ||
| path: . | ||
|
|
||
| - 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 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,202 @@ | ||
| #!/usr/bin/env bash | ||
| # | ||
| # sso-audit.sh — Outline-side fork audit against the cross-app SSO contract. | ||
| # | ||
| # ============================================================================ | ||
| # Covers fork-side rows of awais786/sso-rules-moneta:openspec/specs/ | ||
| # proxy-auth-middleware/spec.md that a deterministic bash check can verify | ||
| # against Outline's source tree. Catches regressions BEFORE they reach | ||
| # foss-server-bundle-devstack. | ||
| # | ||
| # Exit codes: | ||
| # 0 — all rows ✅ or n/a / informational | ||
| # 1 — at least one SECURITY-CRITICAL row failed | ||
| # | ||
| # Rows covered: | ||
| # Row 14 — logout shape: SPA logout (app/stores/AuthStore.ts) MUST NOT | ||
| # call /oauth2/sign_out. | ||
| # Row 20 — session-identity reconciliation (Rule 2 mismatch flush): | ||
| # server/middlewares/authentication.ts MUST throw | ||
| # AuthenticationError on identity mismatch when AUTH_TYPE === 'SSO' | ||
| # and transport === 'cookie'. The existing outer auth() catch | ||
| # block clears the accessToken cookie via err.headers. | ||
| # SECURITY-CRITICAL. | ||
| # Row 21 — email-shape detection MUST NOT use polynomial-backtracking | ||
| # regex. Outline uses indexOf-based detection in | ||
| # normalizeProxyEmail (post-PR #19); this row is a regression | ||
| # guard. | ||
| # ============================================================================ | ||
|
|
||
| set -euo pipefail | ||
|
|
||
| AUTH_MIDDLEWARE="server/middlewares/authentication.ts" | ||
| AUTH_STORE="app/stores/AuthStore.ts" | ||
|
|
||
| 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 indexOf, not polynomial regex" | ||
| ) | ||
| declare -a ROW_NOTES=() | ||
| declare -a ROW_NUMBERS=(14 20 21) | ||
|
|
||
| SECURITY_CRITICAL=(1) | ||
| 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 | ||
| } | ||
|
|
||
| # ============================================================================ | ||
| # Row 14 (idx 0): logout shape — narrow check | ||
| # ============================================================================ | ||
| check_row_14() { | ||
| if [[ ! -f "$AUTH_STORE" ]]; then | ||
| record 0 "?" "$AUTH_STORE not found — skipping" | ||
| return | ||
| fi | ||
|
|
||
| if grep -qE '/oauth2/sign_?out' "$AUTH_STORE"; then | ||
| local line | ||
| line=$(grep -nE '/oauth2/sign_?out' "$AUTH_STORE" | head -1) | ||
| record 0 "❌" "Logout calls \`/oauth2/sign_out\` at $AUTH_STORE:$line — per logout-flow spec, per-app Logout is navigation-only. Drop the call; portal 'Logout all' handles oauth2-proxy clearing." | ||
| return | ||
| fi | ||
|
|
||
| record 0 "✅" "$AUTH_STORE 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)" | ||
| } | ||
|
|
||
| # ============================================================================ | ||
| # Row 20 (idx 1): session-identity reconciliation | ||
| # | ||
| # Outline's flush mechanism is: throw AuthenticationError → outer auth() | ||
| # catch block clears the accessToken cookie via err.headers. The deterministic | ||
| # signal is the presence of the `normalizeProxyEmail` helper function — it | ||
| # was added in Pressingly/outline#19 specifically to do the bidirectional | ||
| # header normalisation for the mismatch comparison. The function only exists | ||
| # post-fix, so its presence is a precise marker. | ||
| # | ||
| # A weaker fallback check looks for the "proxy identity" literal in a | ||
| # `throw AuthenticationError(...)` call, in case a future refactor renames | ||
| # the helper. | ||
| # | ||
| # The test suite at server/middlewares/authentication.test.ts pins the | ||
| # exact behaviour with the `should clear stale accessToken cookie when | ||
| # proxy identity changes` and `should NOT clear cookie when proxy email | ||
| # matches JWT user` test cases. | ||
| # | ||
| # SECURITY-CRITICAL: without this, the stale-session leak returns. | ||
| # ============================================================================ | ||
| check_row_20() { | ||
| if [[ ! -f "$AUTH_MIDDLEWARE" ]]; then | ||
| record 1 "?" "$AUTH_MIDDLEWARE not found — skipping" | ||
| return | ||
| fi | ||
|
|
||
| # Primary marker: normalizeProxyEmail function (introduced by PR #19). | ||
| local has_normalize_helper | ||
| has_normalize_helper=$(grep -cF "normalizeProxyEmail" "$AUTH_MIDDLEWARE" || true) | ||
|
|
||
| # Fallback marker: "proxy identity" literal in a throw — used in the | ||
| # AuthenticationError message when the mismatch fires. | ||
| local has_throw_proxy | ||
| has_throw_proxy=$(grep -cE "throw[[:space:]]+AuthenticationError\(.*[Pp]roxy" "$AUTH_MIDDLEWARE" || true) | ||
|
|
||
| if [[ "$has_normalize_helper" -gt 0 ]]; then | ||
| record 1 "✅" "$AUTH_MIDDLEWARE defines \`normalizeProxyEmail\` and uses it for bidirectional header normalisation — Rule 2 mismatch flush in place (flush mechanism: throw 401 → outer auth() catch clears accessToken cookie)" | ||
| return | ||
| fi | ||
|
|
||
| if [[ "$has_throw_proxy" -gt 0 ]]; then | ||
| record 1 "✅" "$AUTH_MIDDLEWARE has \`throw AuthenticationError(...proxy...)\` indicating a stale-session detection — Rule 2 mismatch flush in place (consider extracting bidirectional normalisation into a normalizeProxyEmail helper for clarity)" | ||
| return | ||
| fi | ||
|
|
||
| record 1 "❌" "$AUTH_MIDDLEWARE does NOT define \`normalizeProxyEmail\` and has no \`throw AuthenticationError(...proxy...)\` call site. The cross-app spec (proxy-auth-middleware Rule 2) requires the JWT cookie branch of validateAuthentication to compare normalized X-Auth-Request-Email against the JWT user's email and throw AuthenticationError on mismatch, gated on \`env.AUTH_TYPE === 'SSO'\` and \`transport === 'cookie'\`. The existing outer auth() catch block then clears the accessToken + lastSignedIn cookies via err.headers. Without this check, the stale-session-on-user-switch leak returns. Reference: Pressingly/outline#19." | ||
| } | ||
|
|
||
| # ============================================================================ | ||
| # Row 21 (idx 2): polynomial-regex avoidance in email-shape detection | ||
| # | ||
| # Detects the problematic pattern [^\s@]+@ which causes polynomial | ||
| # backtracking. The pattern matches character classes that exclude both | ||
| # whitespace (\s) and @ symbols, followed by +@ — this is the exact | ||
| # construct that leads to catastrophic backtracking in email validation. | ||
| # | ||
| # Note: The grep pattern '\[\^\\s@\]\+@' searches for the literal string | ||
| # "[^\s@]+@" in the source code (where \s is a JavaScript regex token, | ||
| # not a grep pattern). We're escaping the brackets and + to match them | ||
| # literally in the source. | ||
| # | ||
| # Using head -1 ensures only the first match is reported, preventing | ||
| # newlines from breaking the markdown table output. | ||
| # ============================================================================ | ||
| check_row_21() { | ||
| if [[ ! -f "$AUTH_MIDDLEWARE" ]]; then | ||
| record 2 "?" "$AUTH_MIDDLEWARE not found — skipping" | ||
| return | ||
| fi | ||
|
|
||
| local hits | ||
| hits=$(grep -nE '\[\^\\s@\]\+@' "$AUTH_MIDDLEWARE" 2>/dev/null | head -1 || true) | ||
|
|
||
| if [[ -n "$hits" ]]; then | ||
| record 2 "❌" "Polynomial-backtracking email-shape regex detected in $AUTH_MIDDLEWARE: $hits. Rewrite to indexOf-based check per openspec proxy-auth-middleware §'email-shape detection SHALL avoid polynomial-backtracking regex'. Reference: \`normalizeProxyEmail\` in this file." | ||
| return | ||
| fi | ||
|
|
||
| record 2 "✅" "No polynomial-backtracking email-shape regex in $AUTH_MIDDLEWARE; using indexOf-based detection" | ||
|
Comment on lines
+151
to
+159
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified locally against the current branch state — the audit regex does match line 316 of $ grep -nE '\[\^[\\\\]s@\]\+@\[\^[\\\\]s@\]\+\\\.\[\^[\\\\]s@\]\+' server/middlewares/authentication.ts
316: const isValidEmail = /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(emailClaim);And So:
This finding is real and should not be removed from the audit. Closing this thread; the underlying ReDoS gap on the |
||
| } | ||
|
|
||
| # ============================================================================ | ||
| # Run checks | ||
| # ============================================================================ | ||
| check_row_14 | ||
| check_row_20 | ||
| check_row_21 | ||
|
|
||
| # ============================================================================ | ||
| # Print table | ||
| # ============================================================================ | ||
| echo "## Outline 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 | ||
| printf "| %d | %s | %s | %s |\n" \ | ||
| "${ROW_NUMBERS[$i]}" "${ROW_TITLES[$i]}" "${ROW_STATUS[$i]:-?}" "${ROW_NOTES[$i]:-}" | ||
| 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 | ||
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 6b26a1c. I scoped workflow permissions to least privilege:
pull-requests: writewas removed from workflow-level permissions and moved to a separate PR-onlysso-fork-audit-commentjob, while the main audit job now runs with read-only permissions and passes output via artifact.