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.

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

Component-level data fetching functions included in client bundle #1615

Closed
coreyasmith opened this issue Sep 21, 2023 · 5 comments · Fixed by #1753
Closed

Component-level data fetching functions included in client bundle #1615

coreyasmith opened this issue Sep 21, 2023 · 5 comments · Fixed by #1753
Labels
backlog Issue/PR/discussion is reviewed and added to backlog for the further work 🐞 bug

Comments

@coreyasmith
Copy link
Contributor

coreyasmith commented Sep 21, 2023

Describe the Bug

getStaticProps or getServerSideProps are included in the client-side bundle when they are defined at the component level. Any imports they depend on are included in the client-side bundle as well, even if the imports are not used by any client-side code. Consider the following:

type ContentBlockProps = {
  dbData: string;
  serverData: string;
};

const ContentBlock = ({ dbData, serverData }: ContentBlockProps): JSX.Element => {
  return (
    <div className="contentBlock">
      <h2>{dbData}</h2>
      <h2>{serverData}</h2>
    </div>
  );
};

import { getFromDatabase } from './database';
import serverData from './server-data.json';

export const getStaticProps = async () => {
  const dbConnectionString = 'secret-connection-string';
  const dbData = getFromDatabase(dbConnectionString);

  return {
    dbData,
    serverData,
  };
};

export default ContentBlock;

In this example, the entire getStaticProps function will be included in the client-side bundle, leaking secrets to the browser. The ./database and ./server-data.json imports will be included in the client bundle, inflating the client bundle with code that will not be used.

This is a bug because if the same code were declared in a getStaticProps function defined at the page level:

  1. The getStaticProps function would be excluded from the client-side bundle;
  2. The imports that getStaticProps depends on would be tree-shaken and excluded from the client-side bundle.

The Next.js documentation for getStaticProps and getServerSideProps both state that code in these functions will be eliminated from the client-side bundle when defined at the page level.

Next.js developers will expect the same behavior from the JSS component-level data fetching implementation. This reasonable assumption will at best lead to bloated client-side bundles, and at worst lead to leaked secrets. Vercel's next-code-elimination tool even suggests that the getStaticProps function in the example above will be eliminated from the client-side bundle; however, that is not actually the case.

To Reproduce

Implement the component described above. Search within the client-side bundle at http://localhost:3000/_next/static/chunks/pages/%5B%5B...path%5D%5D.js for getStaticProps or secret-connection-string; observe that they are included in the client-side bundle. Search for the imports used by getStaticProps and note that they are included in the client-side bundle as well.

Implement the same code in src\pages\[[...path]].tsx's getStaticProps method. Search within the client bundle http://localhost:3000/_next/static/chunks/pages/%5B%5B...path%5D%5D.js for getStaticProps or secret-connection-string. Observe that they are excluded from the client-side bundle. Search for the imports used by getStaticProps and note that they are excluded from the client-side bundle.

Expected Behavior

Component-level data fetching functions getServerSideProps and getStaticProps behave the same as when they are defined at the page level:

  1. Component-level data fetching functions are excluded from client-side bundles;
  2. Imports used only by component-level data fetching functions are tree-shaken and excluded from client-side bundles.

Possible Fix

An environment variable, IS_SERVER, could be defined with webpack in the Next config that evaluates to 'true' on the server and 'false' on the client:

// src\lib\next-config\plugins\component-props.js

/**
 * @param {import('next').NextConfig} nextConfig
 */
const componentPropsPlugin = (nextConfig = {}) => {
  return Object.assign({}, nextConfig, {
    webpack: (config, options) => {
      config.plugins.push(
        new options.webpack.DefinePlugin({
          'process.env.IS_SERVER': JSON.stringify(options.isServer.toString()),
        })
      );

      // Overload the Webpack config if it was already overloaded
      if (typeof nextConfig.webpack === 'function') {
        return nextConfig.webpack(config, options);
      }

      return config;
    }
  });
};

module.exports = componentPropsPlugin;

Component-level getStaticProps calls and its imports could then be nested into a process.env.IS_SERVER === 'true' check and webpack will exclude them from the client-side bundle:

type ContentBlockProps = {
  dbData: string;
  serverData: string;
};

const ContentBlock = ({ dbData, serverData }: ContentBlockProps): JSX.Element => {
  return (
    <div className="contentBlock">
      <h2>{dbData}</h2>
      <h2>{serverData}</h2>
    </div>
  );
};

if (process.env.IS_SERVER === 'true') {
  const database = import('./database');
  const serverData = import('./server-data.json');

  module.exports.getStaticProps = async () => {
    const dbConnectionString = 'secret-connection-string';
    const dbData = (await database).getFromDatabase(dbConnectionString);

    return {
      dbData,
      serverData: (await serverData).default,
    };
  };
}

export default ContentBlock;

However this is more of a workaround than a fix. A true fix would handle this automatically and not require developer intervention.

Perhaps updating the Component Builder or Module Factory to not import getServerSideProps or getStaticProps when IS_SERVER evaluates to true would handle this.

Provide environment information

@illiakovalenko illiakovalenko added the backlog Issue/PR/discussion is reviewed and added to backlog for the further work label Sep 25, 2023
@illiakovalenko
Copy link
Contributor

@coreyasmith Thanks for the detailed information. We have a ticket in our internal backlog and will work on this issue.

@chrissnyder2337
Copy link

Subscribing... This would be beneficial for my project.

@chrissnyder2337
Copy link

Subscribing... This would be beneficial for my project.

I want to add more information on how beneficial this would be for XM Cloud projects like mine. Preventing secrets and other sensitive information from being included in the client-side bundle when used for component-level data fetching would allow us to create components that depend on authenticated API requests. This would allow us to create SXA components that request information from other systems that content editors can place on ANY page. Currently, this is not possible without exposing API credentials, which is a huge security issue.

@matthewgisonno
Copy link

111% ... Merkle, Inc. checking in here... This seems like a major flaw to a majorly cool feature. We have components which use component-level data fetching with getStaticProps making graphql calls to Sitecore. This works fine since the Sitecore API key is not a secret.

But we have a current client that has their own API, which does use a secret, making this component level data fetching impossible. Instead, I had to move the data fetching into our pages route handler and pass the data down to components via react context, which is not ideal, but works.

@matthewgisonno
Copy link

I will also add, @coreyasmith has a Possible Fix above... It was almost there, but I couldn't get it working, for whatever reason. I did get something working though, which does reliably strip out all the extra imported code and secrets from the client bundle. @coreyasmith appreciate your research around this.

I will note, when running this locally and not built, some of the code still shows as in the client bundle. Once the code is built, there is no remnants of any secret or code imported / used in component-level getStaticProps anywhere in the client bundle. Screenshots attached at the end.

This function gets attached to the webpack configuration in next.config.js:

/**
 * This function modifies the Webpack configuration to define a global environment variable 'IS_SERVER'.
 * It utilizes the DefinePlugin to inject 'process.env.IS_SERVER' with the stringified boolean value indicating
 * if the current build context is server-side. This allows for conditional code paths in the application
 * based on whether the code is being executed on the server or the client.
 *
 * @param {object} config - The current webpack config object to be modified.
 * @param {object} options - An options object provided by Next.js which includes a boolean 'isServer' property.
 */
function addIsServerEnvVariable(config, options) {
  config.plugins.push(
    new options.webpack.DefinePlugin({
      'process.env.IS_SERVER': JSON.stringify(options.isServer.toString())
    })
  );
}

.env.local or Vercel environment variable:
SUPER_SECRET_OF_ALL_SECRETS=111177142114771111

In your component, you will add the getStaticProps with server check:

// Component-level getStaticProps: Aggregated by Sitecore JSS at the page level.
// IS_SERVER check to remove items from client bundle.
export const getStaticProps = async () => {
  if (process.env.IS_SERVER === 'true') {
    const SUPER_SECRET_STUFF = process.env.SUPER_SECRET_OF_ALL_SECRETS;
    console.log(
      'console log it since unused variables are stripped, SUPER_SECRET_STUFF:',
      SUPER_SECRET_STUFF
    );

    // Dynamic Import
    const { getProgramCoursesData } = await import(
      '@/lib/services/programGraphQL/programCourses'
    );

    // Execute Sitecore GraphQL call
    const { courses = {} } = await getProgramCoursesData();

    // Data returned here is accessed in the component via useComponentProps hook using the rendering UID.
    return { courses };
  }
};

Screenshot 2024-02-08 at 4 17 59 PM
Screenshot 2024-02-08 at 4 18 21 PM
Screenshot 2024-02-08 at 4 18 52 PM
Screenshot 2024-02-08 at 4 19 57 PM
Screenshot 2024-02-08 at 4 22 58 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Issue/PR/discussion is reviewed and added to backlog for the further work 🐞 bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants