diff --git a/docs/error-handling.md b/docs/error-handling.md index 2545145b3..51e031cd3 100644 --- a/docs/error-handling.md +++ b/docs/error-handling.md @@ -29,7 +29,7 @@ Central HTTP UX lives in `src/app/core/interceptors/error.interceptor.ts`. Prefe Order before the status `switch` matters: -1. **SSR (`!isBrowser`)** — no toast, navigation, loader, auth, or Sentry; rethrow only. +1. **SSR (`!isBrowser`)** — no toast, navigation, loader, auth, or Sentry. For non-bypassed `401` / `403` / `404` / `410` / `5xx`, set `RESPONSE_INIT.status` on the requested URL; then rethrow. Network/timeout errors (`status: 0`) map to `503`. First non-OK wins, except a later `401` / `403` / `404` / `410` may replace an earlier `5xx`. Silent requests (`X-No-Auth-Redirect` or request-access URL) do not set status for any code; pending-moderation `403`s do. GUID guards (`isFileGuard`, `isProjectGuard`, `isRegistryGuard`) catch errors and return `false`, so matching falls through to `**` (not-found page HTML with the HTTP status from `RESPONSE_INIT`). 2. **Bypass** — Sentry capture + rethrow; skip all UX. 3. **Maintenance** — `503` with `meta.maintenance_mode: true` → hide loader, activate maintenance mode, rethrow. 4. **Status `switch`** — `401` / `403` / `404` / `409` / `410` / default. @@ -39,7 +39,7 @@ After the `switch` (except early `409` return), the interceptor hides the loader ```mermaid flowchart TD err[HttpErrorResponse] --> ssr{Browser?} - ssr -->|no| rethrow[Rethrow] + ssr -->|no| ssrStatus[Set RESPONSE_INIT status + rethrow] ssr -->|yes| bypass{BYPASS?} bypass -->|yes| sentry[Sentry + rethrow] bypass -->|no| maint{Maintenance 503?} @@ -49,7 +49,7 @@ flowchart TD status -->|other| rethrow2[Hide loader + rethrow] rethrow2 --> state[NGXS catchError / handleSectionError] conflict --> state - rethrow --> state + ssrStatus --> state sentry --> state activate --> state ``` @@ -65,7 +65,7 @@ flowchart TD | **410** | Navigate to `/content-deleted`. Hide loader. No toast. | | **Other** | Toast via `getErrorMessage`, hide loader. | -`/not-found` and `/content-deleted` share `PageNotFoundComponent` with different route data (`PAGE_NOT_FOUND_ROUTE_DATA` / `CONTENT_DELETED_ROUTE_DATA`). +`/not-found` and `/content-deleted` share `PageNotFoundComponent` with different route data (`PAGE_NOT_FOUND_ROUTE_DATA` / `CONTENT_DELETED_ROUTE_DATA`). Browser navigates to those routes (and `/forbidden`) for UX; SSR does not — it only sets `RESPONSE_INIT.status` on the requested URL via `ssr-response-status.helper.ts`. ### Bypass @@ -151,6 +151,7 @@ Guidelines: | ------------------------------------------------------- | ------------------------------------------ | | `src/app/core/interceptors/error.interceptor.ts` | HTTP UX handler | | `src/app/core/interceptors/error-interceptor.tokens.ts` | Bypass token + factory | +| `src/app/core/helpers/ssr-response-status.helper.ts` | SSR `RESPONSE_INIT.status` helper | | `src/app/shared/helpers/state-error.handler.ts` | NGXS section error helper | | `src/app/shared/models/store/async-state.model.ts` | Async section shape (`error` field) | | `src/app/core/constants/error-messages.ts` | Status → i18n keys | diff --git a/docs/ssr.md b/docs/ssr.md index 47b8ba14e..58d93bf5d 100644 --- a/docs/ssr.md +++ b/docs/ssr.md @@ -153,6 +153,10 @@ Docker `start:docker` runs the **development** configuration (CSR), not the SSR --- +## HTTP status codes for failed resources + +When SSR fetches a resource and the API returns `401` / `403` / `404` / `410` / `5xx`, the error interceptor sets `RESPONSE_INIT.status` on the requested URL. Network/timeout errors (`status: 0`) map to `503`. First non-OK wins, except a later `401` / `403` / `404` / `410` may replace an earlier `5xx`. No navigation/toasts on the server. Bypass skips this. Silent requests (`X-No-Auth-Redirect` or request-access URL) do not set status for any code. Anonymous SSR usually sees `401` for private resources (not `403`). GUID guards catch errors and return `false`, so routing falls through to `**` (not-found page HTML with that HTTP status). See [Error handling](./error-handling.md). + ## Adding or changing SSR routes 1. Add the route in `app.routes.ts` (browser routes). diff --git a/src/app/core/guards/is-file.guard.ts b/src/app/core/guards/is-file.guard.ts index 482e257f4..76eabbf44 100644 --- a/src/app/core/guards/is-file.guard.ts +++ b/src/app/core/guards/is-file.guard.ts @@ -1,6 +1,6 @@ import { Store } from '@ngxs/store'; -import { map, switchMap } from 'rxjs/operators'; +import { catchError, map, of, switchMap } from 'rxjs'; import { isPlatformBrowser } from '@angular/common'; import { inject, PLATFORM_ID } from '@angular/core'; @@ -58,6 +58,7 @@ export const isFileGuard: CanMatchFn = (route: Route, segments: UrlSegment[]) => } return resource.type === CurrentResourceType.Files; - }) + }), + catchError(() => of(false)) ); }; diff --git a/src/app/core/guards/is-project.guard.ts b/src/app/core/guards/is-project.guard.ts index d3b3e11c3..69638b3ed 100644 --- a/src/app/core/guards/is-project.guard.ts +++ b/src/app/core/guards/is-project.guard.ts @@ -1,6 +1,6 @@ import { Store } from '@ngxs/store'; -import { map, switchMap } from 'rxjs/operators'; +import { catchError, map, of, switchMap } from 'rxjs'; import { inject } from '@angular/core'; import { CanMatchFn, Route, Router, UrlSegment } from '@angular/router'; @@ -49,6 +49,7 @@ export const isProjectGuard: CanMatchFn = (route: Route, segments: UrlSegment[]) } return resource.type === CurrentResourceType.Projects; - }) + }), + catchError(() => of(false)) ); }; diff --git a/src/app/core/guards/is-registry.guard.ts b/src/app/core/guards/is-registry.guard.ts index db23c3f8f..f531ca78d 100644 --- a/src/app/core/guards/is-registry.guard.ts +++ b/src/app/core/guards/is-registry.guard.ts @@ -1,6 +1,6 @@ import { Store } from '@ngxs/store'; -import { map, switchMap } from 'rxjs/operators'; +import { catchError, map, of, switchMap } from 'rxjs'; import { inject } from '@angular/core'; import { CanMatchFn, Route, Router, UrlSegment } from '@angular/router'; @@ -49,6 +49,7 @@ export const isRegistryGuard: CanMatchFn = (route: Route, segments: UrlSegment[] } return resource.type === CurrentResourceType.Registrations; - }) + }), + catchError(() => of(false)) ); }; diff --git a/src/app/core/helpers/ssr-response-status.helper.spec.ts b/src/app/core/helpers/ssr-response-status.helper.spec.ts new file mode 100644 index 000000000..4e1baad10 --- /dev/null +++ b/src/app/core/helpers/ssr-response-status.helper.spec.ts @@ -0,0 +1,46 @@ +import { setSsrResponseStatus, shouldPropagateHttpStatusToSsr } from './ssr-response-status.helper'; + +describe('ssr-response-status.helper', () => { + it('should propagate 0, 401, 403, 404, 410, and 5xx statuses', () => { + expect(shouldPropagateHttpStatusToSsr(0)).toBe(true); + expect(shouldPropagateHttpStatusToSsr(401)).toBe(true); + expect(shouldPropagateHttpStatusToSsr(403)).toBe(true); + expect(shouldPropagateHttpStatusToSsr(404)).toBe(true); + expect(shouldPropagateHttpStatusToSsr(410)).toBe(true); + expect(shouldPropagateHttpStatusToSsr(500)).toBe(true); + expect(shouldPropagateHttpStatusToSsr(503)).toBe(true); + }); + + it('should not propagate 409 or other 4xx statuses', () => { + expect(shouldPropagateHttpStatusToSsr(409)).toBe(false); + expect(shouldPropagateHttpStatusToSsr(422)).toBe(false); + }); + + it('should set status when response init is available and unset', () => { + const responseInit: ResponseInit = {}; + setSsrResponseStatus(responseInit, 404); + expect(responseInit.status).toBe(404); + }); + + it('should map network status 0 to 503', () => { + const responseInit: ResponseInit = {}; + setSsrResponseStatus(responseInit, 0); + expect(responseInit.status).toBe(503); + }); + + it('should keep an existing non-OK status when the next status is 5xx', () => { + const responseInit: ResponseInit = { status: 404 }; + setSsrResponseStatus(responseInit, 500); + expect(responseInit.status).toBe(404); + }); + + it('should replace an earlier 5xx with a later resource status', () => { + const responseInit: ResponseInit = { status: 503 }; + setSsrResponseStatus(responseInit, 404); + expect(responseInit.status).toBe(404); + }); + + it('should no-op when response init is missing', () => { + expect(() => setSsrResponseStatus(null, 404)).not.toThrow(); + }); +}); diff --git a/src/app/core/helpers/ssr-response-status.helper.ts b/src/app/core/helpers/ssr-response-status.helper.ts new file mode 100644 index 000000000..87deec39c --- /dev/null +++ b/src/app/core/helpers/ssr-response-status.helper.ts @@ -0,0 +1,23 @@ +export const isServerError = (status: number): boolean => status >= 500 && status < 600; + +const isResourceHttpStatus = (status: number): boolean => + status === 401 || status === 403 || status === 404 || status === 410; + +const toSsrHttpStatus = (status: number): number => (status === 0 ? 503 : status); + +export function setSsrResponseStatus(responseInit: ResponseInit | null, status: number): void { + if (!responseInit) { + return; + } + + const next = toSsrHttpStatus(status); + const current = responseInit.status; + + if (!current || current === 200 || (isServerError(current) && isResourceHttpStatus(next))) { + responseInit.status = next; + } +} + +export function shouldPropagateHttpStatusToSsr(status: number): boolean { + return status === 0 || isResourceHttpStatus(status) || isServerError(status); +} diff --git a/src/app/core/interceptors/error.interceptor.spec.ts b/src/app/core/interceptors/error.interceptor.spec.ts index 423e1417a..951e22394 100644 --- a/src/app/core/interceptors/error.interceptor.spec.ts +++ b/src/app/core/interceptors/error.interceptor.spec.ts @@ -3,7 +3,7 @@ import { MockProvider } from 'ng-mocks'; import { firstValueFrom, throwError } from 'rxjs'; import { HttpContext, HttpErrorResponse, HttpHeaders, HttpRequest } from '@angular/common/http'; -import { PLATFORM_ID } from '@angular/core'; +import { PLATFORM_ID, RESPONSE_INIT } from '@angular/core'; import { TestBed } from '@angular/core/testing'; import { Router } from '@angular/router'; @@ -40,6 +40,7 @@ describe('errorInterceptor', () => { let maintenanceModeServiceMock: MaintenanceModeServiceMockType; let viewOnlyHelperMock: ViewOnlyLinkHelperMockType; let sentryMock: SentryMockType; + let responseInit: ResponseInit; function setup( platformId: 'browser' | 'server' = 'browser', @@ -54,6 +55,7 @@ describe('errorInterceptor', () => { maintenanceModeServiceMock = MaintenanceModeServiceMock.simple(); viewOnlyHelperMock = ViewOnlyLinkHelperMock.simple(viewOnly); sentryMock = SentryMock.simple(); + responseInit = {}; TestBed.configureTestingModule({ providers: [ @@ -69,6 +71,7 @@ describe('errorInterceptor', () => { MockProvider(ViewOnlyLinkHelperService, viewOnlyHelperMock), MockProvider(PLATFORM_ID, platformId), { provide: SENTRY_TOKEN, useValue: sentryMock }, + { provide: RESPONSE_INIT, useValue: responseInit }, ], }); } @@ -115,19 +118,118 @@ describe('errorInterceptor', () => { expect(toastServiceMock.showError).toHaveBeenCalledWith('server failed'); }); - it('should rethrow without handling on server platform', async () => { + it('should set SSR response status without navigation on server platform', async () => { setup('server'); const request = createRequest('/api/v2/nodes/abc'); - const error = new HttpErrorResponse({ status: 500, error: { message: 'server failed' }, url: request.url }); + const error = new HttpErrorResponse({ status: 404, error: {}, url: request.url }); const caught = await runInterceptor(request, error); - expect(caught?.status).toBe(500); + expect(caught?.status).toBe(404); + expect(responseInit.status).toBe(404); + expect(router.navigate).not.toHaveBeenCalled(); expect(loaderServiceMock.hide).not.toHaveBeenCalled(); expect(toastServiceMock.showError).not.toHaveBeenCalled(); expect(sentryMock.captureException).not.toHaveBeenCalled(); }); + it('should set SSR response status for 401 without navigation', async () => { + setup('server'); + const request = createRequest('/api/v2/guids/abc'); + const error = new HttpErrorResponse({ status: 401, error: {}, url: request.url }); + + const caught = await runInterceptor(request, error); + + expect(caught?.status).toBe(401); + expect(responseInit.status).toBe(401); + expect(router.navigate).not.toHaveBeenCalled(); + }); + + it('should set SSR response status for pending-moderation 403', async () => { + setup('server'); + const request = createRequest('/api/v2/preprints/abc'); + const error = new HttpErrorResponse({ + status: 403, + error: { errors: [{ detail: PREPRINT_PENDING_MODERATION_DETAIL }] }, + url: request.url, + }); + + const caught = await runInterceptor(request, error); + + expect(caught?.status).toBe(403); + expect(responseInit.status).toBe(403); + expect(router.navigate).not.toHaveBeenCalled(); + }); + + it('should not set SSR response status for silent 403', async () => { + setup('server'); + const request = createRequest('/api/v2/nodes/abc', false, true); + const error = new HttpErrorResponse({ status: 403, error: {}, url: request.url }); + + const caught = await runInterceptor(request, error); + + expect(caught?.status).toBe(403); + expect(responseInit.status).toBeUndefined(); + }); + + it('should not set SSR response status for bypassed requests on server platform', async () => { + setup('server'); + const request = createRequest('/api/v2/nodes/abc', true); + const error = new HttpErrorResponse({ status: 404, error: {}, url: request.url }); + + const caught = await runInterceptor(request, error); + + expect(caught?.status).toBe(404); + expect(responseInit.status).toBeUndefined(); + }); + + it('should keep the first non-OK SSR response status', async () => { + setup('server'); + responseInit.status = 404; + + const request = createRequest('/api/v2/nodes/abc'); + const error = new HttpErrorResponse({ status: 500, error: {}, url: request.url }); + + await runInterceptor(request, error); + + expect(responseInit.status).toBe(404); + }); + + it('should replace an earlier 5xx SSR response status with a resource status', async () => { + setup('server'); + responseInit.status = 503; + + const request = createRequest('/api/v2/nodes/abc'); + const error = new HttpErrorResponse({ status: 404, error: {}, url: request.url }); + + await runInterceptor(request, error); + + expect(responseInit.status).toBe(404); + }); + + it('should map network status 0 to 503 on server platform', async () => { + setup('server'); + const request = createRequest('/api/v2/nodes/abc'); + const error = new HttpErrorResponse({ status: 0, error: {}, url: request.url }); + + const caught = await runInterceptor(request, error); + + expect(caught?.status).toBe(0); + expect(responseInit.status).toBe(503); + expect(router.navigate).not.toHaveBeenCalled(); + }); + + it('should not set SSR response status for silent network errors', async () => { + setup('server'); + const request = createRequest('/api/v2/nodes/abc', false, true); + const error = new HttpErrorResponse({ status: 0, error: {}, url: request.url }); + + const caught = await runInterceptor(request, error); + + expect(caught?.status).toBe(0); + expect(responseInit.status).toBeUndefined(); + }); + it('should rethrow 409 without toast or loader handling', async () => { setup(); const request = createRequest('/api/v2/nodes/abc'); diff --git a/src/app/core/interceptors/error.interceptor.ts b/src/app/core/interceptors/error.interceptor.ts index c4ac11379..1709438d0 100644 --- a/src/app/core/interceptors/error.interceptor.ts +++ b/src/app/core/interceptors/error.interceptor.ts @@ -4,11 +4,16 @@ import { catchError, throwError } from 'rxjs'; import { isPlatformBrowser } from '@angular/common'; import { HttpErrorResponse, HttpInterceptorFn, HttpRequest } from '@angular/common/http'; -import { inject, PLATFORM_ID } from '@angular/core'; +import { inject, PLATFORM_ID, RESPONSE_INIT } from '@angular/core'; import { Router } from '@angular/router'; import { ERROR_MESSAGES } from '@core/constants/error-messages'; import { PREPRINT_PENDING_MODERATION_DETAIL } from '@core/constants/http-error-details.const'; +import { + isServerError, + setSsrResponseStatus, + shouldPropagateHttpStatusToSsr, +} from '@core/helpers/ssr-response-status.helper'; import { SENTRY_TOKEN } from '@core/provider/sentry.provider'; import { AuthService } from '@core/services/auth.service'; import { MaintenanceModeService } from '@core/services/maintenance-mode.service'; @@ -24,8 +29,6 @@ const REQUEST_ACCESS_URL_REGEX = /\/v2\/nodes\/[^/]+\/requests\/?$/i; const NODE_ID_URL_REGEX = /\/v2\/nodes\/([^/]+)/; const DRAFT_REGISTRY_ROUTE_REGEX = /\/registries\/drafts\/.+/i; -const isServerError = (status: number): boolean => status >= 500 && status < 600; - const isSilentForbidden = (req: HttpRequest, error: HttpErrorResponse): boolean => req.headers.has(NO_AUTH_REDIRECT_HEADER) || (!!error.url && REQUEST_ACCESS_URL_REGEX.test(error.url)); @@ -57,6 +60,7 @@ export const errorInterceptor: HttpInterceptorFn = (req, next) => { const sentry = inject(SENTRY_TOKEN); const viewOnlyHelper = inject(ViewOnlyLinkHelperService); const store = inject(Store); + const responseInit = inject(RESPONSE_INIT, { optional: true }); const isBrowser = isPlatformBrowser(inject(PLATFORM_ID)); const handleUnauthorized = () => { @@ -81,11 +85,24 @@ export const errorInterceptor: HttpInterceptorFn = (req, next) => { } }; + const applySsrStatus = (error: HttpErrorResponse) => { + if ( + req.context.get(BYPASS_ERROR_INTERCEPTOR) || + !shouldPropagateHttpStatusToSsr(error.status) || + isSilentForbidden(req, error) + ) { + return; + } + + setSsrResponseStatus(responseInit, error.status); + }; + return next(req).pipe( catchError((error: HttpErrorResponse) => { const rethrow = () => throwError(() => error); if (!isBrowser) { + applySsrStatus(error); return rethrow(); }