Skip to content
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.

Sign up for GitHub

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

feat: add Content-Security-Policy Header, allows setting frame-ancestor domains and moved font local #975

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
+123 −50
Open

feat: add Content-Security-Policy Header, allows setting frame-ancestor domains and moved font local #975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6019d98
feat: add Content-Security-Policy Header, allows setting frame-ancest…
Ruakij Sep 22, 2024
257cab3
Merge branch 'develop' into feature/content-security-policy-setting_f…
Ruakij Oct 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cypress/config/settings.cypress.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
"applicationTitle": "Overseerr",
"applicationUrl": "",
"csrfProtection": false,
"cspFrameAncestorDomains": "",
"cacheImages": false,
"defaultPermissions": 32,
"defaultQuotas": {
Expand Down
3 changes: 3 additions & 0 deletions overseerr-api.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,9 @@ components:
csrfProtection:
type: boolean
example: false
cspFrameAncestorDomains:
type: string
example: 'example.com'
hideAvailable:
type: boolean
example: false
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
"express-session": "1.17.3",
"formik": "^2.4.6",
"gravatar-url": "3.1.0",
"helmet": "^7.1.0",
"lodash": "4.17.21",
"mime": "3",
"next": "^14.2.4",
Expand Down
9 changes: 9 additions & 0 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 23 additions & 1 deletion server/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import express from 'express';
import * as OpenApiValidator from 'express-openapi-validator';
import type { Store } from 'express-session';
import session from 'express-session';
import helmet from 'helmet';
import next from 'next';
import dns from 'node:dns';
import net from 'node:net';
Expand Down Expand Up @@ -172,6 +173,23 @@ app
});
}

// Setup Content-Security-Policy
server.use(
helmet.contentSecurityPolicy({
useDefaults: false,
directives: {
'default-src':
helmet.contentSecurityPolicy.dangerouslyDisableDefaultSrc,
'frame-ancestors': [
"'self'",
...(settings.main.cspFrameAncestorDomains
? [settings.main.cspFrameAncestorDomains]
: []),
],
},
})
);

// Set up sessions
const sessionRespository = getRepository(Session);
server.use(
Expand All @@ -183,7 +201,11 @@ app
cookie: {
maxAge: 1000 * 60 * 60 * 24 * 30,
httpOnly: true,
sameSite: settings.main.csrfProtection ? 'strict' : 'lax',
sameSite: settings.main.csrfProtection
? 'strict'
: settings.main.cspFrameAncestorDomains
? 'none'
: 'lax',
secure: 'auto',
},
store: new TypeormStore({
Expand Down
2 changes: 2 additions & 0 deletions server/lib/settings/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ export interface MainSettings {
applicationTitle: string;
applicationUrl: string;
csrfProtection: boolean;
cspFrameAncestorDomains: string;
cacheImages: boolean;
defaultPermissions: number;
defaultQuotas: {
Expand Down Expand Up @@ -311,6 +312,7 @@ class Settings {
applicationTitle: 'Jellyseerr',
applicationUrl: '',
csrfProtection: false,
cspFrameAncestorDomains: '',
cacheImages: false,
defaultPermissions: Permission.REQUEST,
defaultQuotas: {
Expand Down
3 changes: 2 additions & 1 deletion server/utils/restartFlag.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ class RestartFlag {
return (
this.settings.csrfProtection !== settings.csrfProtection ||
this.settings.trustProxy !== settings.trustProxy ||
this.settings.httpProxy !== settings.httpProxy
this.settings.httpProxy !== settings.httpProxy ||
this.settings.cspFrameAncestorDomains !== settings.cspFrameAncestorDomains
);
}
}
Expand Down
30 changes: 30 additions & 0 deletions src/components/Settings/SettingsMain/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ const messages = defineMessages('components.Settings.SettingsMain', {
csrfProtectionTip: 'Set external API access to read-only (requires HTTPS)',
csrfProtectionHoverTip:
'Do NOT enable this setting unless you understand what you are doing!',
cspFrameAncestorDomains: 'Frame-Ancestor Domains',
cspFrameAncestorDomainsTip:
'Domains to allow embedding Jellyseer as iframe, object or embed. Incompatible with CSRF-Protection',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you specify here that this is a space-separated list of domains?

Copy link
Author

@Ruakij Ruakij Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are more possibilities to setting this than just space separated as this is directly put into the header.
But maybe we could link to the specification: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/frame-ancestors

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok. That's a good idea yes

cacheImages: 'Enable Image Caching',
cacheImagesTip:
'Cache externally sourced images (requires a significant amount of disk space)',
Expand Down Expand Up @@ -135,6 +138,7 @@ const SettingsMain = () => {
applicationTitle: data?.applicationTitle,
applicationUrl: data?.applicationUrl,
csrfProtection: data?.csrfProtection,
cspFrameAncestorDomains: data?.cspFrameAncestorDomains,
hideAvailable: data?.hideAvailable,
locale: data?.locale ?? 'en',
region: data?.region,
Expand All @@ -157,6 +161,7 @@ const SettingsMain = () => {
applicationTitle: values.applicationTitle,
applicationUrl: values.applicationUrl,
csrfProtection: values.csrfProtection,
cspFrameAncestorDomains: values.cspFrameAncestorDomains,
hideAvailable: values.hideAvailable,
locale: values.locale,
region: values.region,
Expand Down Expand Up @@ -325,6 +330,31 @@ const SettingsMain = () => {
</Tooltip>
</div>
</div>
<div className="form-row">
<label
htmlFor="cspFrameAncestorDomains"
className="text-label"
>
<span className="mr-2">
{intl.formatMessage(messages.cspFrameAncestorDomains)}
</span>
<SettingsBadge badgeType="advanced" className="mr-2" />
<SettingsBadge badgeType="restartRequired" />
<span className="label-tip">
{intl.formatMessage(messages.cspFrameAncestorDomainsTip)}
</span>
</label>
<div className="form-input-area">
<div className="form-input-field">
<Field
id="cspFrameAncestorDomains"
name="cspFrameAncestorDomains"
type="text"
disabled={values.csrfProtection}
/>
</div>
</div>
</div>
<div className="form-row">
<label htmlFor="cacheImages" className="checkbox-label">
<span className="mr-2">
Expand Down
2 changes: 2 additions & 0 deletions src/i18n/locale/en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -874,6 +874,8 @@
"components.Settings.SettingsMain.applicationurl": "Application URL",
"components.Settings.SettingsMain.cacheImages": "Enable Image Caching",
"components.Settings.SettingsMain.cacheImagesTip": "Cache externally sourced images (requires a significant amount of disk space)",
"components.Settings.SettingsMain.cspFrameAncestorDomains": "Frame-Ancestor Domains",
"components.Settings.SettingsMain.cspFrameAncestorDomainsTip": "Domains to allow embedding Jellyseer as iframe, object or embed. Incompatible with CSRF-Protection",
"components.Settings.SettingsMain.csrfProtection": "Enable CSRF Protection",
"components.Settings.SettingsMain.csrfProtectionHoverTip": "Do NOT enable this setting unless you understand what you are doing!",
"components.Settings.SettingsMain.csrfProtectionTip": "Set external API access to read-only (requires HTTPS)",
Expand Down
86 changes: 45 additions & 41 deletions src/pages/_app.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,13 @@ import { MediaServerType } from '@server/constants/server';
import type { PublicSettingsResponse } from '@server/interfaces/api/settingsInterfaces';
import type { AppInitialProps, AppProps } from 'next/app';
import App from 'next/app';
import { Inter } from 'next/font/google';
import Head from 'next/head';
import { useEffect, useState } from 'react';
import { IntlProvider } from 'react-intl';
import { ToastProvider } from 'react-toast-notifications';
import { SWRConfig } from 'swr';
const inter = Inter({ subsets: ['latin'] });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could add a newline after the imports?


// eslint-disable-next-line @typescript-eslint/no-explicit-any
const loadLocaleData = (locale: AvailableLocale): Promise<any> => {
Expand Down Expand Up @@ -137,47 +139,49 @@ const CoreApp: Omit<NextAppComponentType, 'origGetInitialProps'> = ({
}

return (
<SWRConfig
value={{
fetcher: async (resource, init) => {
const res = await fetch(resource, init);
if (!res.ok) throw new Error();
return await res.json();
},
fallback: {
'/api/v1/auth/me': user,
},
}}
>
<LanguageContext.Provider value={{ locale: currentLocale, setLocale }}>
<IntlProvider
locale={currentLocale}
defaultLocale="en"
messages={loadedMessages}
>
<LoadingBar />
<SettingsProvider currentSettings={currentSettings}>
<InteractionProvider>
<ToastProvider components={{ Toast, ToastContainer }}>
<Head>
<title>{currentSettings.applicationTitle}</title>
<meta
name="viewport"
content="initial-scale=1, viewport-fit=cover, width=device-width"
></meta>
<PWAHeader
applicationTitle={currentSettings.applicationTitle}
/>
</Head>
<StatusChecker />
<ServiceWorkerSetup />
<UserContext initialUser={user}>{component}</UserContext>
</ToastProvider>
</InteractionProvider>
</SettingsProvider>
</IntlProvider>
</LanguageContext.Provider>
</SWRConfig>
<main className={inter.className}>
<SWRConfig
value={{
fetcher: async (resource, init) => {
const res = await fetch(resource, init);
if (!res.ok) throw new Error();
return await res.json();
},
fallback: {
'/api/v1/auth/me': user,
},
}}
>
<LanguageContext.Provider value={{ locale: currentLocale, setLocale }}>
<IntlProvider
locale={currentLocale}
defaultLocale="en"
messages={loadedMessages}
>
<LoadingBar />
<SettingsProvider currentSettings={currentSettings}>
<InteractionProvider>
<ToastProvider components={{ Toast, ToastContainer }}>
<Head>
<title>{currentSettings.applicationTitle}</title>
<meta
name="viewport"
content="initial-scale=1, viewport-fit=cover, width=device-width"
></meta>
<PWAHeader
applicationTitle={currentSettings.applicationTitle}
/>
</Head>
<StatusChecker />
<ServiceWorkerSetup />
<UserContext initialUser={user}>{component}</UserContext>
</ToastProvider>
</InteractionProvider>
</SettingsProvider>
</IntlProvider>
</LanguageContext.Provider>
</SWRConfig>
</main>
);
};

Expand Down
8 changes: 1 addition & 7 deletions src/pages/_document.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,7 @@ class MyDocument extends Document {
render(): JSX.Element {
return (
<Html>
<Head>
<link rel="preconnect" href="https://fonts.gstatic.com" />
<link
rel="stylesheet"
href="https://fonts.googleapis.com/css2?family=Inter:[email protected]&display=swap"
/>
</Head>
<Head></Head>
<body>
<Main />
<NextScript />
Expand Down
4 changes: 4 additions & 0 deletions src/styles/globals.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@
}

@layer components {
*:disabled {
opacity: 0.7;
}

Comment on lines +56 to +59
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before, the text-box wasnt visibly disabled apart from not being able to interact with it, so this makes it a bit more visible

Copy link
Collaborator

@gauthier-th gauthier-th Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check that this does not have any other unexpected behavior? It could have undesired effects on some text fields. and other fields too*

We usually do styling using TailwindCSS, inside the class attribute of the elements

.searchbar {
padding-top: env(safe-area-inset-top);
height: calc(4rem + env(safe-area-inset-top));
Expand Down