diff --git a/README.md b/README.md index cc62182b..e0cbb61a 100644 --- a/README.md +++ b/README.md @@ -91,7 +91,7 @@ You can follow the instructions [here](https://github.com/Yooooomi/your_spotify/ | API_ENDPOINT | REQUIRED | The endpoint of your server | | SPOTIFY_PUBLIC | REQUIRED | The public key of your Spotify application (cf [Creating the Spotify Application](#creating-the-spotify-application)) | | SPOTIFY_SECRET | REQUIRED | The secret key of your Spotify application (cf [Creating the Spotify Application](#creating-the-spotify-application)) | -| CORS | _not defined_ | List of comma-separated origin allowed, or _nothing_ to allow any origin | +| CORS | _not defined_ | List of comma-separated origin allowed | | MAX_IMPORT_CACHE_SIZE | Infinite | The maximum element in the cache when importing data from an outside source, more cache means less requests to Spotify, resulting in faster imports | | MONGO_ENDPOINT | mongodb://mongo:27017/your_spotify | The endpoint of the Mongo database, where **mongo** is the name of your service in the compose file | | PORT | 8080 | The port of the server, do not modify if you're using docker | @@ -102,10 +102,9 @@ You can follow the instructions [here](https://github.com/Yooooomi/your_spotify/ ## CORS -You can edit the CORS for the server: - -- `all` will allow every source. +- Not defining it will default to authorize only the `CLIENT_ENDPOINT` origin. - `origin1,origin2` will allow `origin1` and `origin2`. +> If you really want to allow every origin no matter what, you can set the `CORS` value to `i-want-a-security-vulnerability-and-want-to-allow-all-origins`. # Creating the Spotify Application diff --git a/apps/client/public/index.html b/apps/client/public/index.html index d83a2bf3..725e7f9d 100644 --- a/apps/client/public/index.html +++ b/apps/client/public/index.html @@ -1,33 +1,47 @@ - - - - - - - - Your Spotify - - - - - - - - - - - - - - - - - - - - -
- - + + + + + + + + + + + + + Your Spotify + + + + + + + + + + + + + + + + + + + + + +
+ + + \ No newline at end of file diff --git a/apps/client/scripts/run/serve.json b/apps/client/scripts/run/serve.json new file mode 100644 index 00000000..7afcca6c --- /dev/null +++ b/apps/client/scripts/run/serve.json @@ -0,0 +1,17 @@ +{ + "headers": [ + { + "source": "*", + "headers": [ + { + "key": "Content-Security-Policy", + "value": "frame-ancestors 'none';" + }, + { + "key": "X-Content-Type-Options", + "value": "nosniff" + } + ] + } + ] +} diff --git a/apps/client/scripts/run/serve.sh b/apps/client/scripts/run/serve.sh index e33e5b37..ca49cb92 100755 --- a/apps/client/scripts/run/serve.sh +++ b/apps/client/scripts/run/serve.sh @@ -4,4 +4,4 @@ # -l 0.0.0.0 means that it's hosted on all the interfaces # build/ is the output of the package built at build-time -serve -s -l tcp://0.0.0.0:3000 /app/apps/client/build/ \ No newline at end of file +serve -c /app/apps/client/scripts/run/serve.json -s -l tcp://0.0.0.0:3000 /app/apps/client/build/ \ No newline at end of file diff --git a/apps/client/scripts/run/variables.sh b/apps/client/scripts/run/variables.sh index d3c2360e..9da4b061 100644 --- a/apps/client/scripts/run/variables.sh +++ b/apps/client/scripts/run/variables.sh @@ -18,6 +18,14 @@ then # Editing meta image urls sed -i "s;image\" content=\"\(.[^\"]*\);image\" content=\"$API_ENDPOINT/static/your_spotify_1200.png;g" "$VAR_PATH/index.html" + + # Restricting connect-src to API_ENDPOINT with a trailing / + API_ENDPOINT_ENDING_WITH_SLASH=$API_ENDPOINT + if [[ "$API_ENDPOINT_ENDING_WITH_SLASH" != */ ]] + then + API_ENDPOINT_ENDING_WITH_SLASH="$API_ENDPOINT_ENDING_WITH_SLASH/" + fi + sed -i "s#connect-src \(.*\);#connect-src $API_ENDPOINT_ENDING_WITH_SLASH;#g" "$VAR_PATH/index.html" else echo "API_ENDPOINT is not defined, web app won't work" exit 1 diff --git a/apps/server/scripts/run/deprecated.sh b/apps/server/scripts/run/deprecated.sh index 26ba36c0..35383203 100644 --- a/apps/server/scripts/run/deprecated.sh +++ b/apps/server/scripts/run/deprecated.sh @@ -2,5 +2,6 @@ if [ "$CORS" == "all" ] then - echo "Setting CORS to 'all' is not needed anymore, omitting it is the new way of authorizing every origin." + echo "Setting CORS to 'all' is not authorized anymore. To allow all sources, please specify CORS=i-want-a-security-vulnerability-and-want-to-allow-all-origins" + exit 1 fi diff --git a/apps/server/src/app.ts b/apps/server/src/app.ts index 3d572fc9..0462c3a5 100644 --- a/apps/server/src/app.ts +++ b/apps/server/src/app.ts @@ -17,9 +17,13 @@ import { get } from "./tools/env"; import { LogLevelAccepts } from "./tools/logger"; const app = express(); +const ALLOW_ALL_CORS = + "i-want-a-security-vulnerability-and-want-to-allow-all-origins"; -let corsValue = get("CORS")?.split(","); -if (corsValue?.[0] === "all") { +let corsValue: string[] | undefined = get("CORS")?.split(",") ?? [ + new URL(get("CLIENT_ENDPOINT")).origin, +]; +if (corsValue?.[0] === ALLOW_ALL_CORS) { corsValue = undefined; } @@ -31,6 +35,24 @@ app.use( }), ); +app.use((_, res, next) => { + // Apply security headers for the whole backend here + + // Apply a restrictive CSP for the server API just in case. As there isn't any + // HTML content here, "default-src 'none'" is a good deny-all default in case + // an attacker tries something funny. + // "frame-ancestors 'none'" is required because frame-ancestors doesn't fall + // back to default-src and nobody has legitimate business framing the backend. + res.header( + "Content-Security-Policy", + "default-src 'none'; object-src 'none'; frame-ancestors 'none';" + ); + + // Prevent MIME sniffing in browsers + res.header("X-Content-Type-Options", "nosniff"); + next(); +}); + if (LogLevelAccepts("info")) { app.use(morgan("dev")); } diff --git a/apps/server/src/database/queries/privateData.ts b/apps/server/src/database/queries/privateData.ts index a7f9abc8..76f60e7d 100644 --- a/apps/server/src/database/queries/privateData.ts +++ b/apps/server/src/database/queries/privateData.ts @@ -1,8 +1,8 @@ -import { randomUUID } from "crypto"; +import { generateRandomString } from "../../tools/crypto"; import { PrivateDataModel } from "../Models"; export async function createPrivateData() { - await PrivateDataModel.create({ jwtPrivateKey: randomUUID() }); + await PrivateDataModel.create({ jwtPrivateKey: generateRandomString(32) }); } export async function getPrivateData() { diff --git a/apps/server/src/routes/oauth.ts b/apps/server/src/routes/oauth.ts index a39eead0..13f0d30e 100644 --- a/apps/server/src/routes/oauth.ts +++ b/apps/server/src/routes/oauth.ts @@ -1,5 +1,6 @@ -import { Router } from "express"; +import { Request, Response, Router } from "express"; import { sign } from "jsonwebtoken"; +import { z } from "zod"; import { createUser, getUserCount, @@ -10,16 +11,39 @@ import { get, getWithDefault } from "../tools/env"; import { logger } from "../tools/logger"; import { logged, + validating, withGlobalPreferences, withHttpClient, } from "../tools/middleware"; import { Spotify } from "../tools/oauth/Provider"; -import { GlobalPreferencesRequest, SpotifyRequest } from "../tools/types"; +import { + GlobalPreferencesRequest, + SpotifyRequest, + TypedPayload, +} from "../tools/types"; import { getPrivateData } from "../database/queries/privateData"; export const router = Router(); -router.get("/spotify", async (_, res) => { +function storeTokenInCookie( + request: Request, + response: Response, + token: string, +) { + response.cookie("token", token, { + sameSite: "strict", + httpOnly: true, + secure: request.secure, + }); +} + +const OAUTH_COOKIE_NAME = "oauth"; +const spotifyCallbackOAuthCookie = z.object({ + state: z.string(), +}); +type OAuthCookie = z.infer; + +router.get("/spotify", async (req, res) => { const isOffline = get("OFFLINE_DEV_ID"); if (isOffline) { const privateData = await getPrivateData(); @@ -29,52 +53,85 @@ router.get("/spotify", async (_, res) => { const token = sign({ userId: isOffline }, privateData.jwtPrivateKey, { expiresIn: getWithDefault("COOKIE_VALIDITY_MS", "1h"), }); - res.cookie("token", token); + storeTokenInCookie(req, res, token); res.status(204).end(); return; } - res.redirect(Spotify.getRedirect()); + const { url, state } = await Spotify.getRedirect(); + const oauthCookie: OAuthCookie = { + state, + }; + + res.cookie(OAUTH_COOKIE_NAME, oauthCookie, { + sameSite: "lax", + httpOnly: true, + secure: req.secure, + }); + + res.redirect(url); }); -router.get("/spotify/callback", withGlobalPreferences, async (req, res) => { - const { query, globalPreferences } = req as GlobalPreferencesRequest; - const { code } = query; +const spotifyCallback = z.object({ + code: z.string(), + state: z.string(), +}); - const infos = await Spotify.exchangeCode(code as string); +router.get( + "/spotify/callback", + validating(spotifyCallback, "query"), + withGlobalPreferences, + async (req, res) => { + const { query, globalPreferences } = req as GlobalPreferencesRequest; + const { code, state } = query as TypedPayload; - try { - const client = Spotify.getHttpClient(infos.accessToken); - const { data: spotifyMe } = await client.get("/me"); - let user = await getUserFromField("spotifyId", spotifyMe.id, false); - if (!user) { - if (!globalPreferences.allowRegistrations) { - return res.redirect(`${get("CLIENT_ENDPOINT")}/registrations-disabled`); + try { + const cookie = spotifyCallbackOAuthCookie.parse( + req.cookies[OAUTH_COOKIE_NAME], + ); + + if (state !== cookie.state) { + throw new Error("State does not match"); } - const nbUsers = await getUserCount(); - user = await createUser( - spotifyMe.display_name, - spotifyMe.id, - nbUsers === 0, + + const infos = await Spotify.exchangeCode(code, cookie.state); + + const client = Spotify.getHttpClient(infos.accessToken); + const { data: spotifyMe } = await client.get("/me"); + let user = await getUserFromField("spotifyId", spotifyMe.id, false); + if (!user) { + if (!globalPreferences.allowRegistrations) { + return res.redirect( + `${get("CLIENT_ENDPOINT")}/registrations-disabled`, + ); + } + const nbUsers = await getUserCount(); + user = await createUser( + spotifyMe.display_name, + spotifyMe.id, + nbUsers === 0, + ); + } + await storeInUser("_id", user._id, infos); + const privateData = await getPrivateData(); + if (!privateData?.jwtPrivateKey) { + throw new Error("No private data found, cannot sign JWT"); + } + const token = sign( + { userId: user._id.toString() }, + privateData.jwtPrivateKey, + { + expiresIn: getWithDefault("COOKIE_VALIDITY_MS", "1h"), + }, ); + storeTokenInCookie(req, res, token); + } catch (e) { + logger.error(e); + } finally { + res.clearCookie(OAUTH_COOKIE_NAME); } - await storeInUser("_id", user._id, infos); - const privateData = await getPrivateData(); - if (!privateData?.jwtPrivateKey) { - throw new Error("No private data found, cannot sign JWT"); - } - const token = sign( - { userId: user._id.toString() }, - privateData.jwtPrivateKey, - { - expiresIn: getWithDefault("COOKIE_VALIDITY_MS", "1h"), - }, - ); - res.cookie("token", token); - } catch (e) { - logger.error(e); - } - return res.redirect(get("CLIENT_ENDPOINT")); -}); + return res.redirect(get("CLIENT_ENDPOINT")); + }, +); router.get("/spotify/me", logged, withHttpClient, async (req, res) => { const { client } = req as SpotifyRequest; diff --git a/apps/server/src/routes/search.ts b/apps/server/src/routes/search.ts index 5fbe6094..6f8d8d43 100644 --- a/apps/server/src/routes/search.ts +++ b/apps/server/src/routes/search.ts @@ -1,10 +1,10 @@ -import { Router } from 'express'; -import { z } from 'zod'; -import { searchArtist, searchTrack } from '../database'; -import { logger } from '../tools/logger'; -import { isLoggedOrGuest, validating } from '../tools/middleware'; -import { TypedPayload } from '../tools/types'; -import { searchAlbum } from '../database/queries/album'; +import { Router } from "express"; +import { z } from "zod"; +import { searchArtist, searchTrack } from "../database"; +import { logger } from "../tools/logger"; +import { isLoggedOrGuest, validating } from "../tools/middleware"; +import { TypedPayload } from "../tools/types"; +import { searchAlbum } from "../database/queries/album"; export const router = Router(); @@ -23,7 +23,7 @@ router.get( const [artists, tracks, albums] = await Promise.all([ searchArtist(query), searchTrack(query), - searchAlbum(query) + searchAlbum(query), ]); return res.status(200).send({ artists, tracks, albums }); } catch (e) { diff --git a/apps/server/src/tools/crypto.ts b/apps/server/src/tools/crypto.ts new file mode 100644 index 00000000..53a08bed --- /dev/null +++ b/apps/server/src/tools/crypto.ts @@ -0,0 +1,12 @@ +import { getRandomValues, subtle } from "crypto"; + +export function generateRandomString(entropyBytes: number) { + const entropy = getRandomValues(new Uint8Array(entropyBytes)); + return Buffer.from(entropy).toString("base64url"); +} + +export async function sha256(plain: string) { + const encoder = new TextEncoder(); + const data = encoder.encode(plain); + return subtle.digest("SHA-256", data); +} diff --git a/apps/server/src/tools/env.ts b/apps/server/src/tools/env.ts index 89f563cb..dc417b39 100644 --- a/apps/server/src/tools/env.ts +++ b/apps/server/src/tools/env.ts @@ -1,5 +1,4 @@ import { z } from "zod"; -import { logger } from "./logger"; import { toBoolean, toNumber } from "./zod"; const validators = { diff --git a/apps/server/src/tools/oauth/Provider.ts b/apps/server/src/tools/oauth/Provider.ts index 52831a4c..2c7d9017 100644 --- a/apps/server/src/tools/oauth/Provider.ts +++ b/apps/server/src/tools/oauth/Provider.ts @@ -1,12 +1,13 @@ /* eslint-disable @typescript-eslint/no-unused-vars */ import Axios from "axios"; +import { generateRandomString, sha256 } from "../crypto"; import { credentials } from "./credentials"; export class Provider { static getRedirect = () => {}; // @ts-ignore - static exchangeCode = code => {}; + static exchangeCode = (code: string, state: string) => {}; // @ts-ignore static refresh = refreshToken => {}; @@ -19,18 +20,26 @@ export class Provider { } export class Spotify extends Provider { - static getRedirect = () => { + static getRedirect = async () => { const { scopes } = credentials.spotify; const { redirectUri } = credentials.spotify; - return `https://accounts.spotify.com/authorize?response_type=code&client_id=${ - credentials.spotify.public - }${ - scopes ? `&scope=${encodeURIComponent(scopes)}` : "" - }&redirect_uri=${encodeURIComponent(redirectUri)}`; + const authorizeUrl = new URL("https://accounts.spotify.com/authorize"); + const state = generateRandomString(32); + + authorizeUrl.searchParams.append("client_id", credentials.spotify.public); + authorizeUrl.searchParams.append("response_type", "code"); + authorizeUrl.searchParams.append("redirect_uri", redirectUri); + authorizeUrl.searchParams.append("state", state); + authorizeUrl.searchParams.append("scope", scopes); + + return { + url: authorizeUrl.toString(), + state, + }; }; - static exchangeCode = async (code: string) => { + static exchangeCode = async (code: string, state: string) => { const { data } = await Axios.post( "https://accounts.spotify.com/api/token", null, @@ -41,6 +50,7 @@ export class Spotify extends Provider { redirect_uri: credentials.spotify.redirectUri, client_id: credentials.spotify.public, client_secret: credentials.spotify.secret, + state, }, headers: { "Content-Type": "application/x-www-form-urlencoded", diff --git a/monorepo.code-workspace b/monorepo.code-workspace index f032b570..2354d85f 100644 --- a/monorepo.code-workspace +++ b/monorepo.code-workspace @@ -17,6 +17,12 @@ ], "[typescript]": { "editor.defaultFormatter": "dbaeumer.vscode-eslint" + }, + "[json]": { + "editor.defaultFormatter": "vscode.json-language-features" + }, + "[html]": { + "editor.defaultFormatter": "vscode.html-language-features" } }, "extensions": {