fix: Prevent newlines in environment variables from causing frontend syntax errors

[rolodato]

Thanks for submitting a PR! Please check the boxes below:

• I have added information to docs/ if required so people know about the feature!
• I have filled in the "Changes" section below?
• I have filled in the "How did you test this code" section below?
• I have used a Conventional Commit title for this Pull Request

Changes

If a frontend environment variable such as FLAGSMITH_ON_FLAGSMITH_API_KEY includes a trailing newline, the response from /config/project-overrides will be invalid JavaScript. For example, running the frontend like this causes a syntax error on the login page:

rolodato@rolosmith frontend % FLAGSMITH_ON_FLAGSMITH_API_KEY='ENktaJnfLVbLifybz34JmX
' npm run dev

results in this response, which is invalid and causes a frontend syntax error:

window.projectOverrides = {
            preventSignup: false,
                    preventEmailPassword: false,
                    preventForgotPassword: false,
                    superUserCreateOnly: false,
                    flagsmith: 'ENktaJnfLVbLifybz34JmX
',
            maintenance: false,
                    disableAnalytics: false,
                    flagsmithAnalytics: true,
                flagsmithRealtime: false,
                    hideInviteLinks: false,
                    linkedinPartnerTracking: false,
                    useSecureCookies: true,
            
    };

A customer ran into this when using a manually base64-encoded Kubernetes secret without realising it had a trailing newline, which broke their frontend.

String.prototype.trim removes all surrounding whitespace, not just newlines, but I'm 99% certain we will never have environment variables that depend on trailing or leading whitespace.

This can still break if environment variables contain ', /* or other strings that interfere with JS. We should use a smarter mechanism to inject configuration that cannot cause a syntax error in the frontend, such as injecting a JSON payload and trying to decode that from JS, instead of injecting JS directly.

How did you test this code?

Manually by running the FE locally, and confirming this works as expected with this change:

FLAGSMITH_ON_FLAGSMITH_API_KEY='ENktaJnfLVbLifybz34JmX
' npm run dev

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
flagsmith-frontend-preview	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 18, 2024 8:36pm
flagsmith-frontend-staging	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 18, 2024 8:36pm

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
docs	⬜️ Ignored (Inspect)			Oct 18, 2024 8:36pm

[github-actions]

Docker builds report

Image	Build Status	Security report
ghcr.io/flagsmith/flagsmith-api-test:pr-4750	Finished ✅	Skipped
ghcr.io/flagsmith/flagsmith-e2e:pr-4750	Finished ✅	Skipped
ghcr.io/flagsmith/flagsmith-api:pr-4750	Finished ✅	Results ✅
ghcr.io/flagsmith/flagsmith:pr-4750	Finished ✅	Results ✅
ghcr.io/flagsmith/flagsmith-private-cloud:pr-4750	Finished ✅	Results ✅
ghcr.io/flagsmith/flagsmith-frontend:pr-4750	Finished ✅	Results ✅

[github-actions]

Uffizzi Preview deployment-57433 was deleted.

[matthewelwell]

This seems reasonable to me.

Minor question: have we checked the behaviour of the python logic that also generates the same file in the single container image set up? It looks to me (based on this) like it would depend on how the settings attributes are generated, and whether there's any trimming built in there.

[rolodato]

This bug only affects the FE image because it's trying to manually generate valid JavaScript. The backend instead generates a JSON payload, which is guaranteed to be valid JS and no trimming is necessary:

flagsmith/api/app/views.py

Line 69 in fbfe317

content="window.projectOverrides = " + json.dumps(override_data),

.

The failure mode of having unwanted trailing/leading whitespace would be slightly different if using the unified image, as API requests could possibly fail, but at least it won't cause the whole FE to crash.

The FE should probably scrap the current approach of generating JS and instead do the same thing the API is doing, but that would be a more intrusive fix.