forked from twentyhq/twenty
-
Notifications
You must be signed in to change notification settings - Fork 0
chore(ci): add fork-side SSO audit script + workflow #9
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
Closed
Closed
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
6328a2c
chore(ci): add fork-side SSO audit script + workflow
awais786 dd2be34
fix(ci): tighten row 20 audit matching and scope workflow permissions
Copilot 397b288
fix(ci): address review follow-ups for audit matching and artifact path
Copilot 1ae5de3
fix(ci): dedupe row20 patterns and use explicit audit artifact directory
Copilot c874245
fix(ci): use proximity constant and harden artifact upload step
Copilot 51e5e1d
chore(ci): document regex intent and fork-pr comment job scope
Copilot 3ef61b8
chore(ci): simplify regex quoting and awk variable initialization
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,86 @@ | ||
| 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) 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: | ||
| - 'packages/twenty-server/src/engine/guards/**' | ||
| - 'packages/twenty-server/src/engine/core-modules/auth/**' | ||
| - 'packages/twenty-front/src/modules/auth/**' | ||
| - 'scripts/sso-audit.sh' | ||
| - '.github/workflows/sso-audit.yml' | ||
| workflow_dispatch: | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| jobs: | ||
| sso-fork-audit: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - 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-fork-audit-output | ||
| path: audit-output.md | ||
| if-no-files-found: warn | ||
|
|
||
| - 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: | ||
| # Run comment updates only on same-repo PRs. Fork PR tokens are read-only | ||
| # and cannot write PR comments. | ||
| if: | | ||
| always() && | ||
| github.event_name == 'pull_request' && | ||
| github.event.pull_request.head.repo.full_name == github.repository | ||
| needs: sso-fork-audit | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
| steps: | ||
| - name: Download audit output | ||
| uses: actions/download-artifact@v4 | ||
| with: | ||
| name: sso-fork-audit-output | ||
| path: audit-results | ||
|
|
||
| - name: Post sticky PR comment | ||
| continue-on-error: true | ||
| uses: marocchino/sticky-pull-request-comment@v2 | ||
| with: | ||
| header: sso-fork-audit | ||
| path: audit-results/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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,257 @@ | ||
| #!/usr/bin/env bash | ||
| # | ||
| # sso-audit.sh — Twenty-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 Twenty'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 and the per-fork audits in Plane / | ||
| # Outline / Penpot / SurfSense, the cross-app contract is now ~14 of 21 | ||
| # rows deterministic without any LLM invocation. | ||
| # | ||
| # Exit codes: | ||
| # 0 — all rows ✅ or n/a / informational | ||
| # 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 (packages/twenty-front/src/ | ||
| # modules/auth/hooks/useAuth.ts) MUST NOT call /oauth2/sign_out. | ||
| # (The portal-host redirect target uses buildPortalUrl — verified | ||
| # elsewhere; this row only enforces no /oauth2/sign_out literal.) | ||
| # Row 20 — session-identity reconciliation (Rule 2 mismatch flush): | ||
| # JwtAuthGuard MUST call response.clearCookie('tokenPair', ...) | ||
| # on identity mismatch when AUTH_TYPE === 'SSO'. Without this, | ||
| # the stale-session-on-user-switch leak returns. SECURITY-CRITICAL. | ||
| # Row 21 — email-shape detection MUST NOT use polynomial-backtracking | ||
| # regex. Twenty uses indexOf-based detection in | ||
| # normalizeProxyEmail today; this row 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 | ||
|
|
||
| JWT_GUARD="packages/twenty-server/src/engine/guards/jwt-auth.guard.ts" | ||
| USE_AUTH="packages/twenty-front/src/modules/auth/hooks/useAuth.ts" | ||
| # clearCookie('tokenPair', ...) | ||
| CLEAR_COOKIE_PATTERN='clearCookie\(\s*['"'"'"]tokenPair['"'"'"]' | ||
| # get('AUTH_TYPE') === 'SSO' | ||
| AUTH_TYPE_SSO_PATTERN='get\(\s*['"'"'"]AUTH_TYPE['"'"'"]\s*\)\s*===\s*['"'"'"]SSO['"'"'"]' | ||
| MISMATCH_HEADER_PATTERN="X-Auth-Request-Email" | ||
| MISMATCH_USER_PATTERN="data\\.user\\.email" | ||
| # 25 lines keeps the required signals in one logical guard block while | ||
| # tolerating normal formatting/wrapping drift. | ||
| PROXIMITY_WINDOW=25 | ||
|
|
||
| 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 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 | ||
| } | ||
|
|
||
| # ============================================================================ | ||
| # Row 14 (idx 0): logout shape — narrow check | ||
| # | ||
| # SPA logout MUST NOT contain `/oauth2/sign_out` literal. Twenty's logout | ||
| # uses `buildPortalUrl` to compute the redirect target; this audit only | ||
| # verifies the SPA doesn't try to clear the upstream proxy cookie itself. | ||
| # ============================================================================ | ||
| check_row_14() { | ||
| if [[ ! -f "$USE_AUTH" ]]; then | ||
| record 0 "?" "$USE_AUTH not found — skipping" | ||
| return | ||
| fi | ||
|
|
||
| if grep -qE '/oauth2/sign_?out' "$USE_AUTH"; then | ||
| local line | ||
| line=$(grep -nE '/oauth2/sign_?out' "$USE_AUTH" | head -1) | ||
| record 0 "❌" "Logout calls \`/oauth2/sign_out\` at $USE_AUTH:$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 "✅" "$USE_AUTH 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 | ||
| # | ||
| # JwtAuthGuard MUST clear the tokenPair cookie on identity mismatch: | ||
| # response.clearCookie('tokenPair', { path: '/' }) | ||
| # | ||
| # AND it MUST gate the mismatch check on AUTH_TYPE === 'SSO' so non-SSO | ||
| # deployments are unaffected. | ||
| # | ||
| # Deterministic signal: | ||
| # - SSO gate: get('AUTH_TYPE') === 'SSO' | ||
| # - mismatch operands: X-Auth-Request-Email + data.user.email | ||
| # - token flush: clearCookie('tokenPair', ...) | ||
| # All must exist in the same nearby block (within ±PROXIMITY_WINDOW lines), so unrelated | ||
| # AUTH_TYPE/clearCookie references elsewhere in the file cannot false-pass. | ||
| # | ||
| # SECURITY-CRITICAL: without this, the stale-session leak returns. | ||
| # ============================================================================ | ||
| check_row_20() { | ||
| if [[ ! -f "$JWT_GUARD" ]]; then | ||
| record 1 "?" "$JWT_GUARD not found — skipping" | ||
| return | ||
| fi | ||
|
|
||
| local clear_cookie_lines | ||
| clear_cookie_lines=$(grep -nE "$CLEAR_COOKIE_PATTERN" "$JWT_GUARD" || true) | ||
|
|
||
| local auth_type_lines | ||
| auth_type_lines=$(grep -nE "$AUTH_TYPE_SSO_PATTERN" "$JWT_GUARD" || true) | ||
|
|
||
| local mismatch_header_lines | ||
| mismatch_header_lines=$(grep -nE "$MISMATCH_HEADER_PATTERN" "$JWT_GUARD" || true) | ||
|
|
||
| local mismatch_user_lines | ||
| mismatch_user_lines=$(grep -nE "$MISMATCH_USER_PATTERN" "$JWT_GUARD" || true) | ||
|
|
||
| if [[ -n "$clear_cookie_lines" && -n "$auth_type_lines" && -n "$mismatch_header_lines" && -n "$mismatch_user_lines" ]]; then | ||
| local nearby_match | ||
| nearby_match=$( | ||
| awk \ | ||
| -v clearPattern="$CLEAR_COOKIE_PATTERN" \ | ||
| -v authPattern="$AUTH_TYPE_SSO_PATTERN" \ | ||
| -v headerPattern="$MISMATCH_HEADER_PATTERN" \ | ||
| -v userPattern="$MISMATCH_USER_PATTERN" \ | ||
| -v window="$PROXIMITY_WINDOW" ' | ||
| $0 ~ clearPattern { clear[NR]=1 } | ||
| $0 ~ authPattern { auth[NR]=1 } | ||
| $0 ~ headerPattern { header[NR]=1 } | ||
| $0 ~ userPattern { user[NR]=1 } | ||
| END { | ||
| for (line = 1; line <= NR; line++) { | ||
| hasClear = 0 | ||
| hasAuth = 0 | ||
| hasHeader = 0 | ||
| hasUser = 0 | ||
| for (i = line - window; i <= line + window; i++) { | ||
| if (clear[i]) hasClear = 1 | ||
| if (auth[i]) hasAuth = 1 | ||
| if (header[i]) hasHeader = 1 | ||
| if (user[i]) hasUser = 1 | ||
| } | ||
| if (hasClear && hasAuth && hasHeader && hasUser) { | ||
| print 1 | ||
| exit | ||
| } | ||
| } | ||
| print 0 | ||
| } | ||
| ' "$JWT_GUARD" | ||
| ) | ||
|
|
||
| if [[ "$nearby_match" == "1" ]]; then | ||
| record 1 "✅" "$JWT_GUARD contains nearby SSO gate + identity-mismatch comparison + \`clearCookie('tokenPair'\` (within ±$PROXIMITY_WINDOW lines) — Rule 2 mismatch flush in place" | ||
| return | ||
| fi | ||
| fi | ||
|
|
||
| if [[ -z "$clear_cookie_lines" ]]; then | ||
| record 1 "❌" "$JWT_GUARD does NOT call \`response.clearCookie('tokenPair', ...)\`. The cross-app spec (proxy-auth-middleware Rule 2) requires the guard to clear the tokenPair cookie when oauth2-proxy asserts a different identity than the JWT user. Without it, the stale-session-on-user-switch leak returns. Fix: add a comparison of \`X-Auth-Request-Email\` vs \`data.user.email\` after \`validateTokenByRequest\`, and on mismatch call \`response.clearCookie('tokenPair', { path: '/' })\` then return false. Gate the check on \`AUTH_TYPE === 'SSO'\`." | ||
| return | ||
| fi | ||
|
|
||
| record 1 "❌" "$JWT_GUARD has \`clearCookie('tokenPair'\` but missing a nearby full Rule 2 block: \`get('AUTH_TYPE') === 'SSO'\` + \`X-Auth-Request-Email\` vs \`data.user.email\` comparison in the same conditional path. Keep these checks colocated to avoid false passes from unrelated references." | ||
| } | ||
|
|
||
| # ============================================================================ | ||
| # Row 21 (idx 2): polynomial-regex avoidance in email-shape detection | ||
| # | ||
| # Twenty uses indexOf-based detection in normalizeProxyEmail (added in | ||
| # Pressingly/twenty#8). This is a regression guard — fires only if a | ||
| # polynomial-backtracking regex like /^[^\s@]+@[^\s@]+\.[^\s@]+$/ is | ||
| # reintroduced. CodeQL's `js/polynomial-redos` flags it on adversarial input. | ||
| # ============================================================================ | ||
| check_row_21() { | ||
| if [[ ! -f "$JWT_GUARD" ]]; then | ||
| record 2 "?" "$JWT_GUARD not found — skipping" | ||
| return | ||
| fi | ||
|
|
||
| # Look for the canonical polynomial-backtracking shape used in JS regex | ||
| # literals: /^[^\s@]+@[^\s@]+\.[^\s@]+$/ and variations. | ||
| local hits | ||
| hits=$(grep -nE '\[\^[\\\\]s@\]\+@\[\^[\\\\]s@\]\+\\\.\[\^[\\\\]s@\]\+' "$JWT_GUARD" 2>/dev/null || true) | ||
|
|
||
| if [[ -n "$hits" ]]; then | ||
| record 2 "❌" "Polynomial-backtracking email-shape regex detected in $JWT_GUARD: $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 $JWT_GUARD; using indexOf-based detection" | ||
| } | ||
|
|
||
| # ============================================================================ | ||
| # Run checks | ||
| # ============================================================================ | ||
| check_row_14 | ||
| check_row_20 | ||
| check_row_21 | ||
|
|
||
| # ============================================================================ | ||
| # Print table | ||
| # ============================================================================ | ||
| echo "## Twenty 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 |
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.
Addressed — current workflow has
contents: readat workflow level andpull-requests: writescoped to the sticky-comment job only.