also looking at this
fix(auth): send people to their reports, and make a dead redirect fail the build
#34
5 files
+59
-16
Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.
| @@ -264,7 +264,16 @@ jobs: | ||
| 264 | 264 | --retry 5 --retry-delay 5 --retry-all-errors --max-time 30 "$@" |
| 265 | 265 | } |
| 266 | 266 | |
| 267 | for path in / /pricing /features /compare/metabase /docs /sitemap.xml /robots.txt; do | |
| 267 | # /login, /register and /reports are here because the pages a stranger | |
| 268 | # lands on were the only ones checked, and the break that mattered was | |
| 269 | # one page further in: all three bounced to a `/projects` that had been | |
| 270 | # deleted, and every probe above stayed green throughout. | |
| 271 | # | |
| 272 | # 200 is a weak assertion for /reports — it renders for an anonymous | |
| 273 | # caller and lets AuthGuard bounce the browser, so curl sees the shell | |
| 274 | # either way. It still catches the failures that matter here: a route | |
| 275 | # that stopped being registered, and a page that 500s. | |
| 276 | for path in / /pricing /features /compare/metabase /docs /sitemap.xml /robots.txt /login /register /reports; do | |
| 268 | 277 | code=$(probe "https://reportshq.org${path}") |
| 269 | 278 | printf '%-24s %s\n' "$path" "$code" |
| 270 | 279 | [ "$code" = "200" ] || fail=1 |
| @@ -16,7 +16,7 @@ const signedIn = session.user !== null | ||
| 16 | 16 | {{-- Already signed in: bounce before anything paints, the same way the |
| 17 | 17 | guard does in the other direction. --}} |
| 18 | 18 | @if (signedIn) |
| 19 | <script>window.location.replace('/projects')</script> | |
| 19 | <script>window.location.replace('/reports')</script> | |
| 20 | 20 | @endif |
| 21 | 21 | |
| 22 | 22 | <script>document.documentElement.setAttribute('data-session-cookie', {{ cookieName }})</script> |
| @@ -3,7 +3,7 @@ | ||
| 3 | 3 | // |
| 4 | 4 | // The one page in the app that must render for a stranger, so it carries no |
| 5 | 5 | // AuthGuard and no app chrome. It does the opposite check instead: somebody who |
| 6 | // already has a session is sent to their projects rather than shown a form | |
| 6 | // already has a session is sent to their reports rather than shown a form | |
| 7 | 7 | // asking them to do something they have already done. |
| 8 | 8 | import { sessionCookieName, sessionFrom } from '../../app/Support/session' |
| 9 | 9 | |
| @@ -17,7 +17,7 @@ const signedIn = session.user !== null | ||
| 17 | 17 | {{-- Already signed in: bounce before anything paints, the same way the |
| 18 | 18 | guard does in the other direction. --}} |
| 19 | 19 | @if (signedIn) |
| 20 | <script>window.location.replace('/projects')</script> | |
| 20 | <script>window.location.replace('/reports')</script> | |
| 21 | 21 | @endif |
| 22 | 22 | |
| 23 | 23 | <script>document.documentElement.setAttribute('data-session-cookie', {{ cookieName }})</script> |
| @@ -121,7 +121,7 @@ const signedIn = session.user !== null | ||
| 121 | 121 | if (res.ok) { |
| 122 | 122 | // replace, not assign: the back button should not return to a sign-in |
| 123 | 123 | // form the person has already used. |
| 124 | window.location.replace('/projects') | |
| 124 | window.location.replace('/reports') | |
| 125 | 125 | return |
| 126 | 126 | } |
| 127 | 127 | |
| @@ -17,7 +17,7 @@ const signedIn = session.user !== null | ||
| 17 | 17 | {{-- Already signed in: bounce before anything paints, the same way the |
| 18 | 18 | guard does in the other direction. --}} |
| 19 | 19 | @if (signedIn) |
| 20 | <script>window.location.replace('/projects')</script> | |
| 20 | <script>window.location.replace('/reports')</script> | |
| 21 | 21 | @endif |
| 22 | 22 | |
| 23 | 23 | <script>document.documentElement.setAttribute('data-session-cookie', {{ cookieName }})</script> |
| @@ -130,7 +130,7 @@ const signedIn = session.user !== null | ||
| 130 | 130 | if (res.ok) { |
| 131 | 131 | // replace, not assign: the back button should not return to a sign-in |
| 132 | 132 | // form the person has already used. |
| 133 | window.location.replace('/projects') | |
| 133 | window.location.replace('/reports') | |
| 134 | 134 | return |
| 135 | 135 | } |
| 136 | 136 | |
| @@ -7,6 +7,15 @@ | ||
| 7 | 7 | * rendered, the links were styled, and they returned the 404 page when clicked. |
| 8 | 8 | * A dead internal link is invisible to everyone except the person following it. |
| 9 | 9 | * |
| 10 | * A `window.location.replace(...)` counts as a link here, and reading only | |
| 11 | * `href` is how five of them survived: login, register and forgot all bounced | |
| 12 | * to `/projects` for months after the page was deleted, on both the | |
| 13 | * already-signed-in path and the just-signed-in one. Nothing caught it. This | |
| 14 | * file read attributes and skipped scripts; the deploy check probes `/` and the | |
| 15 | * marketing pages and never signs in; and the live site kept answering 200 | |
| 16 | * because the release predated the deletion. The first correct deploy would | |
| 17 | * have sent every person who signed in to a 404. | |
| 18 | * | |
| 10 | 19 | * Resolution mirrors how the app serves things: a view at |
| 11 | 20 | * `resources/views/<path>.stx` or `resources/views/<path>/index.stx`, or a |
| 12 | 21 | * document at `docs/<path>.md`. |
| @@ -27,6 +36,16 @@ function publicTemplates(): string[] { | ||
| 27 | 36 | join(views, 'pricing.stx'), |
| 28 | 37 | join(partials, 'SiteNav.stx'), |
| 29 | 38 | join(partials, 'SiteFooter.stx'), |
| 39 | // The auth pages are public in the sense that matters here: they render for | |
| 40 | // a stranger, and they are where a link is most expensive to get wrong. | |
| 41 | // Somebody following a dead marketing link tries another; somebody bounced | |
| 42 | // to a dead page by signing in has no way forward at all. | |
| 43 | join(views, 'login.stx'), | |
| 44 | join(views, 'register.stx'), | |
| 45 | join(views, 'forgot.stx'), | |
| 46 | join(views, 'reset.stx'), | |
| 47 | join(views, 'account.stx'), | |
| 48 | join(partials, 'AuthGuard.stx'), | |
| 30 | 49 | ] |
| 31 | 50 | |
| 32 | 51 | for (const group of ['features', 'use-cases', 'compare']) { |
| @@ -38,19 +57,34 @@ function publicTemplates(): string[] { | ||
| 38 | 57 | return files |
| 39 | 58 | } |
| 40 | 59 | |
| 41 | /** Static hrefs only. A binding is resolved at render time and cannot be read here. */ | |
| 42 | function hrefsIn(file: string): string[] { | |
| 60 | /** | |
| 61 | * Every internal destination a template names, whether the browser follows it | |
| 62 | * on a click or on load. | |
| 63 | * | |
| 64 | * Static only. A binding is resolved at render time and cannot be read here. | |
| 65 | */ | |
| 66 | function linksIn(file: string): string[] { | |
| 43 | 67 | const source = readFileSync(file, 'utf8') |
| 44 | 68 | const found = new Set<string>() |
| 45 | 69 | |
| 46 | for (const match of source.matchAll(/href="([^"{}]+)"/g)) { | |
| 47 | const href = match[1]! | |
| 70 | const patterns = [ | |
| 71 | /href="([^"{}]+)"/g, | |
| 72 | // A redirect is a link the visitor never chose, which makes a dead one | |
| 73 | // worse rather than better: there is nothing to not-click. | |
| 74 | /location\.replace\(\s*['"]([^'"]+)['"]\s*\)/g, | |
| 75 | /location\.href\s*=\s*['"]([^'"]+)['"]/g, | |
| 76 | ] | |
| 77 | ||
| 78 | for (const pattern of patterns) { | |
| 79 | for (const match of source.matchAll(pattern)) { | |
| 80 | const href = match[1]! | |
| 48 | 81 | |
| 49 | // External, mail and in-page links are somebody else's problem. | |
| 50 | if (href.startsWith('http') || href.startsWith('mailto:') || href.startsWith('#')) | |
| 51 | continue | |
| 82 | // External, mail and in-page links are somebody else's problem. | |
| 83 | if (href.startsWith('http') || href.startsWith('mailto:') || href.startsWith('#')) | |
| 84 | continue | |
| 52 | 85 | |
| 53 | found.add(href.split('#')[0]!.split('?')[0]!) | |
| 86 | found.add(href.split('#')[0]!.split('?')[0]!) | |
| 87 | } | |
| 54 | 88 | } |
| 55 | 89 | |
| 56 | 90 | return [...found] |
| @@ -91,7 +125,7 @@ describe('internal links on the public pages', () => { | ||
| 91 | 125 | const name = file.replace(`${root}/`, '') |
| 92 | 126 | |
| 93 | 127 | it(`${name} links only to pages that exist`, () => { |
| 94 | const dead = hrefsIn(file).filter(href => !resolves(href)) | |
| 128 | const dead = linksIn(file).filter(href => !resolves(href)) | |
| 95 | 129 | |
| 96 | 130 | expect(dead).toEqual([]) |
| 97 | 131 | }) |