ReviewOS
Reload

also looking at this

stacks/reportshq

fix(auth): send people to their reports, and make a dead redirect fail the build

#34
Merged glennmichael123 wants to merge fix/auth-redirects into main
5 files +59 -16
.github/workflows/ci.ymlmodified+10-1
@@ -264,7 +264,16 @@ jobs:
264264 --retry 5 --retry-delay 5 --retry-all-errors --max-time 30 "$@"
265265 }
266266
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
268277 code=$(probe "https://reportshq.org${path}")
269278 printf '%-24s %s\n' "$path" "$code"
270279 [ "$code" = "200" ] || fail=1
resources/views/forgot.stxmodified+1-1
@@ -16,7 +16,7 @@ const signedIn = session.user !== null
1616 {{-- Already signed in: bounce before anything paints, the same way the
1717 guard does in the other direction. --}}
1818 @if (signedIn)
19 <script>window.location.replace('/projects')</script>
19 <script>window.location.replace('/reports')</script>
2020 @endif
2121
2222 <script>document.documentElement.setAttribute('data-session-cookie', {{ cookieName }})</script>
resources/views/login.stxmodified+3-3
@@ -3,7 +3,7 @@
33//
44// The one page in the app that must render for a stranger, so it carries no
55// 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
77// asking them to do something they have already done.
88import { sessionCookieName, sessionFrom } from '../../app/Support/session'
99
@@ -17,7 +17,7 @@ const signedIn = session.user !== null
1717 {{-- Already signed in: bounce before anything paints, the same way the
1818 guard does in the other direction. --}}
1919 @if (signedIn)
20 <script>window.location.replace('/projects')</script>
20 <script>window.location.replace('/reports')</script>
2121 @endif
2222
2323 <script>document.documentElement.setAttribute('data-session-cookie', {{ cookieName }})</script>
@@ -121,7 +121,7 @@ const signedIn = session.user !== null
121121 if (res.ok) {
122122 // replace, not assign: the back button should not return to a sign-in
123123 // form the person has already used.
124 window.location.replace('/projects')
124 window.location.replace('/reports')
125125 return
126126 }
127127
resources/views/register.stxmodified+2-2
@@ -17,7 +17,7 @@ const signedIn = session.user !== null
1717 {{-- Already signed in: bounce before anything paints, the same way the
1818 guard does in the other direction. --}}
1919 @if (signedIn)
20 <script>window.location.replace('/projects')</script>
20 <script>window.location.replace('/reports')</script>
2121 @endif
2222
2323 <script>document.documentElement.setAttribute('data-session-cookie', {{ cookieName }})</script>
@@ -130,7 +130,7 @@ const signedIn = session.user !== null
130130 if (res.ok) {
131131 // replace, not assign: the back button should not return to a sign-in
132132 // form the person has already used.
133 window.location.replace('/projects')
133 window.location.replace('/reports')
134134 return
135135 }
136136
tests/feature/marketing-links.test.tsmodified+43-9
@@ -7,6 +7,15 @@
77 * rendered, the links were styled, and they returned the 404 page when clicked.
88 * A dead internal link is invisible to everyone except the person following it.
99 *
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 *
1019 * Resolution mirrors how the app serves things: a view at
1120 * `resources/views/<path>.stx` or `resources/views/<path>/index.stx`, or a
1221 * document at `docs/<path>.md`.
@@ -27,6 +36,16 @@ function publicTemplates(): string[] {
2736 join(views, 'pricing.stx'),
2837 join(partials, 'SiteNav.stx'),
2938 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'),
3049 ]
3150
3251 for (const group of ['features', 'use-cases', 'compare']) {
@@ -38,19 +57,34 @@ function publicTemplates(): string[] {
3857 return files
3958}
4059
41/** Static hrefs only. A binding is resolved at render time and cannot be read here. */
42function 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 */
66function linksIn(file: string): string[] {
4367 const source = readFileSync(file, 'utf8')
4468 const found = new Set<string>()
4569
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]!
4881
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
5285
53 found.add(href.split('#')[0]!.split('?')[0]!)
86 found.add(href.split('#')[0]!.split('?')[0]!)
87 }
5488 }
5589
5690 return [...found]
@@ -91,7 +125,7 @@ describe('internal links on the public pages', () => {
91125 const name = file.replace(`${root}/`, '')
92126
93127 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))
95129
96130 expect(dead).toEqual([])
97131 })