Merge pull request from GHSA-hfgf-99p3-6fjj

* More secure OAuth, fixed frame ancestors

* Change generateRandomString to return base64-encoded entropy

While the old implementation also seems to yield correct results, it is
(at least in my opinion) not obvious that it is correct and sufficiently
secure for the following reasons:

1. It takes the output string length as a parameter instead of the
   required amount of entropy.
   For cryptographic applications, it is important to know how much
   entropy any generated secret contains. Therefore, the input to the
   function should be the required bytes of entropy instead of the
   output string length.

2. While it uses a rather simple algorithm to convert the entropy bytes
   to a string, using a well-known and dead simple encoding such as
   base64 (or hexadecimal, but base64 is more compact) is even easier to
   verify as "obviously correct". In this case URL-safe base64 is chosen
   because the values are also used in URLs.

All usages of the function were also changed to accomodate the parameter
change to requested bytes of entropy. For all of them 32 bytes (256
bits) of entropy were chosen, which nicely fits the purpose of being
used for SHA256 and is definitely enough entropy for things like OAuth
'state'.

* Apply more restrictive CSP and prevent MIME sniffing for backend

As the backend doesn't show HTML pages to users, a very restrictive CSP
can be used. I'm currently not aware of any active issues, but being as
restrictive as possible doesn't hurt, which is what default-src 'none'
does.

Also, set "X-Content-Type-Options: nosniff" to prevent MIME sniffing in
browsers, which can also be an issue in certain circumstances.

* Implement a basic Content-Security-Policy in the client

This commit implements a basic CSP in the client by setting it as a
<meta> tag in index.html. Normally it would be preferred to set the CSP
in a HTTP header, but the react dev server does not seem to support
that. However, it is very important to have the CSP active during
development to catch errors caused by a restrictive CSP before deploying
to production.

Note that a header-based CSP is still required to set
"frame-ancestors 'none'" to prevent clickjacking. It is perfectly fine
to set the CSP both in the <meta> tag and in the HTTP header, both CSPs
will be applied.

There is still room for improvement in the CSP. With some effort, the
script-src could be further restricted using a combination of hashes or
nonces and 'strict-dynamic' to get rid of 'self'. The connect-src could
also be restricted to only the backend origin.

That being said, this is still a very good starting point and much
better than not having a CSP at all.

* Prevent MIME sniffing for the client in production

Prevent MIME sniffing for the client in production by setting the header
"X-Content-Type-Options: nosniff".

* Use only the origin of CLIENT_ENDPOINT for CORS policy

When the full CLIENT_ENDPOINT is used to set the CORS policy, the policy
does not get applied correctly when the client is served under a
subdirectory. Always using the origin fixes this.

* removed pkce, connect-src is dynamic

* ensuring API_ENDPOINT ends with a /

* make sure api endpoint ends with a slash, updated CORS notice

* fixed comment on index.html

---------

Co-authored-by: Marius Renner <marius@mariusrenner.de>

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
 

apps/client/public/index.html

@@ -1,33 +1,47 @@
 <!DOCTYPE html>
 <html lang="en">
-  <head>
-    <meta charset="utf-8" />
-    <link rel="icon" href="%PUBLIC_URL%/favicon.ico" />
-    <link rel="apple-touch-icon" href="%PUBLIC_URL%/logo192.png" />
-    <link rel="manifest" href="%PUBLIC_URL%/manifest.json" />
-    <script src="%PUBLIC_URL%/variables.js"></script>
-
-    <title>Your Spotify</title>
-    <meta name="viewport" content="width=device-width, initial-scale=1" />
-    <meta name="theme-color" content="#000000" />
-
-    <meta name="title" content="Your Spotify">
-    <meta name="description" content="Keep track of your Spotify listening habits with Your Spotify.">
-
-    <meta property="og:type" content="image/png">
-    <meta property="og:title" content="Your Spotify">
-    <meta property="og:image:width" content="1200">
-    <meta property="og:image:height" content="628">
-    <meta property="og:description" content="Keep track of your Spotify listening habits with Your Spotify.">
-    <meta property="og:image" content="http://localhost:8080/static/your_spotify_1200.png">
-
-    <meta property="twitter:card" content="summary">
-    <meta property="twitter:title" content="Your Spotify">
-    <meta property="twitter:description" content="Keep track of your Spotify listening habits with Your Spotify.">
-    <meta property="twitter:image" content="http://localhost:8080/static/your_spotify_1200.png">
-  </head>
-  <body>
-    <noscript>You need to enable JavaScript to run this app.</noscript>
-    <div id="root"></div>
-  </body>
-</html>
+
+<head>
+  <meta charset="utf-8" />
+
+  <!--
+    While setting the CSP in HTTP headers is preferred, there does not seem to
+    be a good way to do this in the react dev environment, so it is set here as
+    a meta tag. Note that frame-ancestors CANNOT be set in the meta tag and
+    MUST be set as a HTTP header in production!
+
+    Restricting connect-src is done at start of the client server.
+    -->
+  <meta http-equiv="Content-Security-Policy" content="default-src 'self'; object-src 'none'; script-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' https://i.scdn.co; connect-src *;" />
+
+  <link rel="icon" href="%PUBLIC_URL%/favicon.ico" />
+  <link rel="apple-touch-icon" href="%PUBLIC_URL%/logo192.png" />
+  <link rel="manifest" href="%PUBLIC_URL%/manifest.json" />
+  <script src="%PUBLIC_URL%/variables.js"></script>
+
+  <title>Your Spotify</title>
+  <meta name="viewport" content="width=device-width, initial-scale=1" />
+  <meta name="theme-color" content="#000000" />
+
+  <meta name="title" content="Your Spotify">
+  <meta name="description" content="Keep track of your Spotify listening habits with Your Spotify.">
+
+  <meta property="og:type" content="image/png">
+  <meta property="og:title" content="Your Spotify">
+  <meta property="og:image:width" content="1200">
+  <meta property="og:image:height" content="628">
+  <meta property="og:description" content="Keep track of your Spotify listening habits with Your Spotify.">
+  <meta property="og:image" content="http://localhost:8080/static/your_spotify_1200.png">
+
+  <meta property="twitter:card" content="summary">
+  <meta property="twitter:title" content="Your Spotify">
+  <meta property="twitter:description" content="Keep track of your Spotify listening habits with Your Spotify.">
+  <meta property="twitter:image" content="http://localhost:8080/static/your_spotify_1200.png">
+</head>
+
+<body>
+  <noscript>You need to enable JavaScript to run this app.</noscript>
+  <div id="root"></div>
+</body>
+
+</html>

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"
+        }
+      ]
+    }
+  ]
+}

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/
+serve -c /app/apps/client/scripts/run/serve.json -s -l tcp://0.0.0.0:3000 /app/apps/client/build/

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

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

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"));
 }

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() {

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<typeof spotifyCallbackOAuthCookie>;
+
+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<typeof spotifyCallback>;
 
-  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;