-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat(Infra) if a file was moved, ensure there is a redirect #15389
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
Open
sergical
wants to merge
14
commits into
master
Choose a base branch
from
sergical/check-redirects-on-rename
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+908
−1
Open
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
90b277a
feat(Infra) if a file was moved, ensure there is a redirect
sergical 8820d3a
fix action
sergical 1e6b873
address comments
sergical a1aed75
update logic
sergical 919971a
robot fixes
sergical a169410
switch back
sergical 71e34fe
safe json
sergical 499aa34
regex escape patterns
sergical 52a252a
[getsentry/action-github-commit] Auto commit
getsantry[bot] 3420c5e
fixing order check
sergical 655394b
[getsentry/action-github-commit] Auto commit
getsantry[bot] 860464f
fix Misparsed Escaped Quotes
sergical e4af32f
Merge branch 'sergical/check-redirects-on-rename' of github.com:getse…
sergical ea5e239
refactor: simplify redirect parser by using require() instead of manu…
sergical 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,141 @@ | ||
| name: Check Redirects on File Rename | ||
|
|
||
| on: | ||
| pull_request: | ||
| branches: [master] | ||
|
|
||
| jobs: | ||
| check-redirects: | ||
| name: Check redirects for renamed files | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
|
|
||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 # Need full history for git diff | ||
|
|
||
| - name: Install bun | ||
| uses: oven-sh/setup-bun@v2 | ||
| with: | ||
| bun-version: latest | ||
|
|
||
| - name: Set up git for diff | ||
| run: | | ||
| git fetch origin ${{ github.event.pull_request.base.ref }}:${{ github.event.pull_request.base.ref }} | ||
| echo "GITHUB_BASE_REF=${{ github.event.pull_request.base.ref }}" >> $GITHUB_ENV | ||
| echo "GITHUB_BASE_SHA=$(git rev-parse origin/${{ github.event.pull_request.base.ref }})" >> $GITHUB_ENV | ||
| echo "GITHUB_SHA=${{ github.event.pull_request.head.sha }}" >> $GITHUB_ENV | ||
|
|
||
| - name: Run redirect validation | ||
| id: validate | ||
| continue-on-error: true | ||
| run: | | ||
| set +e | ||
| OUTPUT=$(bun scripts/check-redirects-on-rename.ts 2>&1) | ||
| EXIT_CODE=$? | ||
| set -e | ||
|
|
||
| echo "$OUTPUT" | ||
|
|
||
| # Extract JSON output if present | ||
| if echo "$OUTPUT" | grep -q "---JSON_OUTPUT---"; then | ||
| JSON_OUTPUT=$(echo "$OUTPUT" | sed -n '/---JSON_OUTPUT---/,/---JSON_OUTPUT---/p' | sed '1d;$d') | ||
| echo "validation_result<<EOF" >> $GITHUB_OUTPUT | ||
| echo "$JSON_OUTPUT" >> $GITHUB_OUTPUT | ||
| echo "EOF" >> $GITHUB_OUTPUT | ||
| echo "has_results=true" >> $GITHUB_OUTPUT | ||
| else | ||
| echo "has_results=false" >> $GITHUB_OUTPUT | ||
| fi | ||
|
|
||
| # Save exit code | ||
| echo "exit_code=$EXIT_CODE" >> $GITHUB_OUTPUT | ||
|
|
||
| - name: Post comment if redirects are missing | ||
| if: steps.validate.outputs.exit_code == '1' && steps.validate.outputs.has_results == 'true' | ||
| uses: actions/github-script@v7 | ||
| with: | ||
| script: | | ||
| const validationResultJson = `${{ steps.validate.outputs.validation_result }}`; | ||
| let validationResult; | ||
| try { | ||
| validationResult = JSON.parse(validationResultJson); | ||
| } catch (e) { | ||
| console.error('Failed to parse validation result:', e); | ||
| return; | ||
| } | ||
|
|
||
| const missingRedirects = validationResult.missingRedirects || []; | ||
|
|
||
| if (missingRedirects.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| // Group by redirects array type | ||
| const devDocsRedirects = missingRedirects.filter(mr => mr.isDeveloperDocs); | ||
| const userDocsRedirects = missingRedirects.filter(mr => !mr.isDeveloperDocs); | ||
|
|
||
| let comment = '## ⚠️ Missing Redirects Detected\n\n'; | ||
| comment += 'This PR renames or moves MDX files, but some redirects may be missing from `redirects.js`.\n\n'; | ||
| comment += 'Please add the following redirects to ensure old URLs continue to work:\n\n'; | ||
|
|
||
| if (userDocsRedirects.length > 0) { | ||
| comment += '### User Docs Redirects (userDocsRedirects array)\n\n'; | ||
| comment += '```javascript\n'; | ||
| userDocsRedirects.forEach(mr => { | ||
| comment += ` {\n`; | ||
| comment += ` source: '${mr.oldUrl}',\n`; | ||
| comment += ` destination: '${mr.newUrl}',\n`; | ||
| comment += ` },\n`; | ||
| }); | ||
| comment += '```\n\n'; | ||
| } | ||
|
|
||
| if (devDocsRedirects.length > 0) { | ||
| comment += '### Developer Docs Redirects (developerDocsRedirects array)\n\n'; | ||
| comment += '```javascript\n'; | ||
| devDocsRedirects.forEach(mr => { | ||
| comment += ` {\n`; | ||
| comment += ` source: '${mr.oldUrl}',\n`; | ||
| comment += ` destination: '${mr.newUrl}',\n`; | ||
| comment += ` },\n`; | ||
| }); | ||
| comment += '```\n\n'; | ||
| } | ||
|
|
||
| comment += '---\n'; | ||
| comment += '_Note: This is a warning and will not block your PR. However, adding redirects ensures old links continue to work._\n'; | ||
|
|
||
| // Check for existing comments from this action | ||
| const {data: comments} = await github.rest.issues.listComments({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: context.issue.number, | ||
| }); | ||
|
|
||
| const existingComment = comments.find(comment => | ||
| comment.user.type === 'Bot' && | ||
| comment.body.includes('Missing Redirects Detected') | ||
| ); | ||
|
|
||
| if (existingComment) { | ||
| // Update existing comment | ||
| await github.rest.issues.updateComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| comment_id: existingComment.id, | ||
| body: comment, | ||
| }); | ||
| } else { | ||
| // Create new comment | ||
| await github.rest.issues.createComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: context.issue.number, | ||
| body: comment, | ||
| }); | ||
| } | ||
File renamed without changes.
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,171 @@ | ||
| import fs from 'fs'; | ||
| import path from 'path'; | ||
|
|
||
| import {afterEach, beforeEach, describe, expect, it} from 'vitest'; | ||
|
|
||
| import { | ||
| filePathToUrls, | ||
| parseRedirectsJs, | ||
| redirectMatches, | ||
| } from './check-redirects-on-rename'; | ||
|
|
||
| // Mock redirects fixture | ||
| const mockRedirectsJs = ` | ||
| const isDeveloperDocs = !!process.env.NEXT_PUBLIC_DEVELOPER_DOCS; | ||
|
|
||
| const developerDocsRedirects = [ | ||
| { | ||
| source: '/sdk/old-path/', | ||
| destination: '/sdk/new-path/', | ||
| }, | ||
| ]; | ||
|
|
||
| const userDocsRedirects = [ | ||
| { | ||
| source: '/platforms/javascript/old-guide/', | ||
| destination: '/platforms/javascript/new-guide/', | ||
| }, | ||
| { | ||
| source: '/platforms/python/old-tutorial', | ||
| destination: '/platforms/python/new-tutorial', | ||
| }, | ||
| ]; | ||
| `; | ||
|
|
||
| describe('filePathToUrls', () => { | ||
| it('should convert docs file path to URLs', () => { | ||
| const result = filePathToUrls('docs/platforms/javascript/index.mdx'); | ||
| expect(result.isDeveloperDocs).toBe(false); | ||
| expect(result.urls).toContain('/platforms/javascript/'); | ||
| expect(result.urls).toContain('/platforms/javascript'); | ||
| }); | ||
|
|
||
| it('should convert develop-docs file path to URLs', () => { | ||
| const result = filePathToUrls('develop-docs/backend/api/index.mdx'); | ||
| expect(result.isDeveloperDocs).toBe(true); | ||
| expect(result.urls).toContain('/backend/api/'); | ||
| expect(result.urls).toContain('/backend/api'); | ||
| }); | ||
|
|
||
| it('should handle non-index files', () => { | ||
| const result = filePathToUrls('docs/platforms/javascript/guide.mdx'); | ||
| expect(result.isDeveloperDocs).toBe(false); | ||
| expect(result.urls).toContain('/platforms/javascript/guide'); | ||
| expect(result.urls).toContain('/platforms/javascript/guide/'); | ||
| }); | ||
|
|
||
| it('should return empty for paths outside docs/develop-docs', () => { | ||
| const result = filePathToUrls('scripts/something.mdx'); | ||
| expect(result.isDeveloperDocs).toBe(false); | ||
| expect(result.urls).toEqual([]); | ||
| }); | ||
| }); | ||
|
|
||
| describe('parseRedirectsJs', () => { | ||
| let tempFile: string; | ||
|
|
||
| beforeEach(() => { | ||
| tempFile = path.join(process.cwd(), 'redirects-test-temp.js'); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| if (fs.existsSync(tempFile)) { | ||
| fs.unlinkSync(tempFile); | ||
| } | ||
| }); | ||
|
|
||
| it('should parse developer docs redirects', () => { | ||
| fs.writeFileSync(tempFile, mockRedirectsJs); | ||
| const result = parseRedirectsJs(tempFile); | ||
| expect(result.developerDocsRedirects).toHaveLength(1); | ||
| expect(result.developerDocsRedirects[0].source).toBe('/sdk/old-path/'); | ||
| expect(result.developerDocsRedirects[0].destination).toBe('/sdk/new-path/'); | ||
| }); | ||
|
|
||
| it('should parse user docs redirects', () => { | ||
| fs.writeFileSync(tempFile, mockRedirectsJs); | ||
| const result = parseRedirectsJs(tempFile); | ||
| expect(result.userDocsRedirects).toHaveLength(2); | ||
| expect(result.userDocsRedirects[0].source).toBe('/platforms/javascript/old-guide/'); | ||
| expect(result.userDocsRedirects[0].destination).toBe( | ||
| '/platforms/javascript/new-guide/' | ||
| ); | ||
| }); | ||
|
|
||
| it('should return empty arrays for non-existent file', () => { | ||
| const result = parseRedirectsJs('/nonexistent/file.js'); | ||
| expect(result.developerDocsRedirects).toEqual([]); | ||
| expect(result.userDocsRedirects).toEqual([]); | ||
| }); | ||
|
|
||
| it('should parse real redirects.js file', () => { | ||
| const result = parseRedirectsJs('redirects.js'); | ||
| // Should have some redirects | ||
| expect(result.developerDocsRedirects.length).toBeGreaterThan(0); | ||
| expect(result.userDocsRedirects.length).toBeGreaterThan(0); | ||
| }); | ||
| }); | ||
|
|
||
| describe('redirectMatches', () => { | ||
| it('should match exact redirects', () => { | ||
| const redirect = { | ||
| source: '/old/path/', | ||
| destination: '/new/path/', | ||
| }; | ||
| expect(redirectMatches(redirect, '/old/path/', '/new/path/')).toBe(true); | ||
| expect(redirectMatches(redirect, '/different/path/', '/new/path/')).toBe(false); | ||
| }); | ||
|
|
||
| it('should match redirects with path parameters', () => { | ||
| const redirect = { | ||
| source: '/platforms/:platform/old/:path*', | ||
| destination: '/platforms/:platform/new/:path*', | ||
| }; | ||
| expect( | ||
| redirectMatches( | ||
| redirect, | ||
| '/platforms/javascript/old/guide', | ||
| '/platforms/javascript/new/guide' | ||
| ) | ||
| ).toBe(true); | ||
| expect( | ||
| redirectMatches( | ||
| redirect, | ||
| '/platforms/python/old/tutorial/', | ||
| '/platforms/python/new/tutorial/' | ||
| ) | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| it('should handle redirects with single path parameter', () => { | ||
| const redirect = { | ||
| source: '/platforms/:platform/old', | ||
| destination: '/platforms/:platform/new', | ||
| }; | ||
| expect( | ||
| redirectMatches(redirect, '/platforms/javascript/old', '/platforms/javascript/new') | ||
| ).toBe(true); | ||
| expect( | ||
| redirectMatches(redirect, '/platforms/python/old', '/platforms/python/new') | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| it('should not match when source pattern does not match', () => { | ||
| const redirect = { | ||
| source: '/platforms/:platform/old', | ||
| destination: '/platforms/:platform/new', | ||
| }; | ||
| expect( | ||
| redirectMatches(redirect, '/different/path', '/platforms/javascript/new') | ||
| ).toBe(false); | ||
| }); | ||
|
|
||
| it('should match destination with exact path when no params', () => { | ||
| const redirect = { | ||
| source: '/old/path', | ||
| destination: '/new/exact/path', | ||
| }; | ||
| expect(redirectMatches(redirect, '/old/path', '/new/exact/path')).toBe(true); | ||
| expect(redirectMatches(redirect, '/old/path', '/different/path')).toBe(false); | ||
| }); | ||
| }); |
Oops, something went wrong.
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.
Bug: Unsafe interpolation of JSON in JS template literals
The
validation_resultJSON is directly interpolated into a JavaScript template literal without escaping. If the JSON contains backticks or template literal syntax, it can cause syntax errors or a code injection vulnerability.