ENG-2895: Add attribution link options to fides-js config#7590
Open
gilluminate wants to merge 6 commits intomainfrom
Open
ENG-2895: Add attribution link options to fides-js config#7590gilluminate wants to merge 6 commits intomainfrom
gilluminate wants to merge 6 commits intomainfrom
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
📝 WalkthroughWalkthroughAdds configurable attribution link options exposed from environment through server settings into the fides-js client bundle: ATTRIBUTION_ENABLED, ATTRIBUTION_ANCHOR_TEXT, ATTRIBUTION_DESTINATION_URL, and ATTRIBUTION_NOFOLLOW, plus types, loaders, API payload inclusion, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Environment Variables
participant Loader as loadEnvironmentVariables()
participant Server as Server Settings
participant API as /api/fides-js
participant Client as Browser / fides-js
Env->>Loader: ATTRIBUTION_* vars
Loader->>Server: returns PrivacyCenterSettings (with attribution fields)
Server->>API: getClientSettings() (maps attribution fields)
API->>API: buildAttributionOptions(settings)
API->>Client: fidesConfig (includes attribution when enabled)
Client->>Client: fides-js reads `fidesConfig.attribution`
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Contributor
Greptile SummaryThis PR adds the server-side config plumbing to deliver a new Key findings:
Confidence Score: 4/5
Important Files Changed
Last reviewed commit: f81479c |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
clients/privacy-center/app/server-utils/loadEnvironmentVariables.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
clients/privacy-center/__tests__/app/fides-js-attribution.test.ts (1)
82-90: Exercise the real handler mapping instead of a mirrored helper.buildAttributionFromSettings()duplicates the production ternary fromclients/privacy-center/pages/api/fides-js.ts, so these tests can stay green even if the handler drifts. I’d rather extract a shared helper or assert against the actual handler response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/privacy-center/__tests__/app/fides-js-attribution.test.ts` around lines 82 - 90, The test helper buildAttributionFromSettings() duplicates the production mapping; replace it by invoking the real handler mapping (or extracting and importing the shared helper used by clients/privacy-center/pages/api/fides-js.ts) so tests assert the actual handler output instead of a mirrored ternary. Locate buildAttributionFromSettings and getClientSettings in the test, remove the duplicated logic, and either import the exported handler/mapping function from clients/privacy-center/pages/api/fides-js.ts (or move the mapping into a new shared function and import it into both the handler and the test), then call that real function and assert its returned AttributionOptions.clients/privacy-center/app/server-utils/loadEnvironmentVariables.ts (1)
200-202: Validate the attribution URL before exposing it to the bundle. This value currently passes straight from env to client-visible config. A typo or non-HTTP(S) scheme will produce a broken or unsafehrefin every rendered footer, so it should fail fast during env loading.Suggested hardening
+const normalizeAttributionDestinationUrl = (value?: string): string => { + const parsed = new URL(value || DEFAULT_ATTRIBUTION_DESTINATION_URL); + if (!["http:", "https:"].includes(parsed.protocol)) { + throw new Error( + "FIDES_PRIVACY_CENTER__ATTRIBUTION_DESTINATION_URL must use http or https", + ); + } + return parsed.toString(); +}; + const loadEnvironmentVariables = () => { // Load environment variables const settings: PrivacyCenterSettings = { @@ - ATTRIBUTION_DESTINATION_URL: - process.env.FIDES_PRIVACY_CENTER__ATTRIBUTION_DESTINATION_URL || - DEFAULT_ATTRIBUTION_DESTINATION_URL, + ATTRIBUTION_DESTINATION_URL: normalizeAttributionDestinationUrl( + process.env.FIDES_PRIVACY_CENTER__ATTRIBUTION_DESTINATION_URL, + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/privacy-center/app/server-utils/loadEnvironmentVariables.ts` around lines 200 - 202, The exported ATTRIBUTION_DESTINATION_URL value is taken directly from process.env and may contain invalid or unsafe schemes; update the environment-loading logic in loadEnvironmentVariables.ts to validate ATTRIBUTION_DESTINATION_URL (falling back to DEFAULT_ATTRIBUTION_DESTINATION_URL) using the URL constructor and ensure the protocol is "http:" or "https:"; if validation fails, throw an error (or exit) during load so the bundle never receives an invalid/unsafe href.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@clients/privacy-center/__tests__/app/fides-js-attribution.test.ts`:
- Around line 82-90: The test helper buildAttributionFromSettings() duplicates
the production mapping; replace it by invoking the real handler mapping (or
extracting and importing the shared helper used by
clients/privacy-center/pages/api/fides-js.ts) so tests assert the actual handler
output instead of a mirrored ternary. Locate buildAttributionFromSettings and
getClientSettings in the test, remove the duplicated logic, and either import
the exported handler/mapping function from
clients/privacy-center/pages/api/fides-js.ts (or move the mapping into a new
shared function and import it into both the handler and the test), then call
that real function and assert its returned AttributionOptions.
In `@clients/privacy-center/app/server-utils/loadEnvironmentVariables.ts`:
- Around line 200-202: The exported ATTRIBUTION_DESTINATION_URL value is taken
directly from process.env and may contain invalid or unsafe schemes; update the
environment-loading logic in loadEnvironmentVariables.ts to validate
ATTRIBUTION_DESTINATION_URL (falling back to
DEFAULT_ATTRIBUTION_DESTINATION_URL) using the URL constructor and ensure the
protocol is "http:" or "https:"; if validation fails, throw an error (or exit)
during load so the bundle never receives an invalid/unsafe href.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6efc97ce-a285-4835-8715-f637d97e9d55
📒 Files selected for processing (8)
changelog/7590-attribution-link-options.yamlclients/fides-js/src/lib/consent-types.tsclients/privacy-center/__tests__/app/fides-js-attribution.test.tsclients/privacy-center/__tests__/server-utils/loadEnvironmentVariables.test.tsclients/privacy-center/app/server-environment.tsclients/privacy-center/app/server-utils/PrivacyCenterSettings.tsclients/privacy-center/app/server-utils/loadEnvironmentVariables.tsclients/privacy-center/pages/api/fides-js.ts
- Extract buildAttributionOptions into shared helper in server-environment.ts - Use shared helper in both fides-js handler and tests (eliminates duplicated logic) - Add URL protocol validation for ATTRIBUTION_DESTINATION_URL Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/privacy-center/app/server-environment.ts`:
- Around line 77-80: The client settings surface is exposing raw ATTRIBUTION_*
env keys; update getClientSettings() (and the similar export around lines
356-359) to stop including "ATTRIBUTION_ENABLED", "ATTRIBUTION_ANCHOR_TEXT",
"ATTRIBUTION_DESTINATION_URL", and "ATTRIBUTION_NOFOLLOW" as top-level
client-facing fields. Instead, inside getClientSettings() build an optional
attribution object server-side only when ATTRIBUTION_ENABLED is truthy (e.g., {
anchorText, destinationUrl, nofollow } sourced from the env) and attach that
object to the returned client settings; omit the attribution field entirely when
disabled. Ensure the raw ATTRIBUTION_* names are removed from the client
contract/type so they are not forwarded to /api/fides-js.
In `@clients/privacy-center/app/server-utils/loadEnvironmentVariables.ts`:
- Around line 58-59: The defaults DEFAULT_ATTRIBUTION_ANCHOR_TEXT and
DEFAULT_ATTRIBUTION_DESTINATION_URL currently contain Ethyca branding; replace
those hard-coded values with generic placeholders (e.g., "Consent powered by
example_org" and "https://example.com/consent" or similar) and ensure any
deployment-specific branding/URL comes from environment variables (retain
existing env var usage around attribution anchor and destination URL so callers
can override at runtime); update DEFAULT_ATTRIBUTION_ANCHOR_TEXT and
DEFAULT_ATTRIBUTION_DESTINATION_URL constants accordingly and keep references to
these symbols (and the env var reads) intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de9a789d-c2fb-4de5-a940-24f63d2bfc38
📒 Files selected for processing (4)
clients/privacy-center/__tests__/app/fides-js-attribution.test.tsclients/privacy-center/app/server-environment.tsclients/privacy-center/app/server-utils/loadEnvironmentVariables.tsclients/privacy-center/pages/api/fides-js.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket ENG-2895
Description Of Changes
Add server-configurable attribution link options to the fides-js bundle. When enabled via
FIDES_PRIVACY_CENTER__ATTRIBUTION_ENABLED=true, anattributionobject is included in theFidesConfig.optionspayload with configurable anchor text, destination URL, and nofollow behavior. When disabled (default), the field is omitted entirely from the serialized config.This is separate from the existing
SHOW_BRAND_LINKsetting - both can be controlled independently.New Environment Variables
FIDES_PRIVACY_CENTER__ATTRIBUTION_ENABLEDfalseFIDES_PRIVACY_CENTER__ATTRIBUTION_ANCHOR_TEXT"Consent powered by Ethyca"FIDES_PRIVACY_CENTER__ATTRIBUTION_DESTINATION_URL"https://ethyca.com/consent"FIDES_PRIVACY_CENTER__ATTRIBUTION_NOFOLLOWfalserel="nofollow"Code Changes
clients/fides-js/src/lib/consent-types.ts- AddAttributionOptionsinterface and optionalattributionfield onFidesInitOptionsclients/privacy-center/app/server-utils/PrivacyCenterSettings.ts- Add 4 new settings:ATTRIBUTION_ENABLED,ATTRIBUTION_ANCHOR_TEXT,ATTRIBUTION_DESTINATION_URL,ATTRIBUTION_NOFOLLOWclients/privacy-center/app/server-utils/loadEnvironmentVariables.ts- Read 4FIDES_PRIVACY_CENTER__ATTRIBUTION_*env vars with sensible defaults; export default constantsclients/privacy-center/app/server-environment.ts- Add attribution fields toPrivacyCenterClientSettingsPick type andgetClientSettings()returnclients/privacy-center/pages/api/fides-js.ts- Conditional inclusion ofattributionobject in FidesConfig options (undefined when disabled)clients/privacy-center/__tests__/app/fides-js-attribution.test.ts- Tests for client settings and config-building logic (9 tests)clients/privacy-center/__tests__/server-utils/loadEnvironmentVariables.test.ts- Tests for env var parsing and defaults (10 tests)Steps to Confirm
Verify default behavior (attribution disabled):
ATTRIBUTION_*env varswindow.Fides.configJSON valueoptions.attributionis not present in the configVerify enabled behavior:
FIDES_PRIVACY_CENTER__ATTRIBUTION_ENABLED=trueoptions.attributionis present with default values:anchorText: "Consent powered by Ethyca"destinationUrl: "https://ethyca.com/consent"nofollow: falseVerify custom overrides:
Pre-Merge Checklist
CHANGELOG.mdupdatedSummary by CodeRabbit
New Features
Tests