Skip to content

Bypass asset_host configuration for icon sprite#5895

Merged
aduth merged 2 commits intomainfrom
aduth-icon-bypass-asset-host
Feb 2, 2022
Merged

Bypass asset_host configuration for icon sprite#5895
aduth merged 2 commits intomainfrom
aduth-icon-bypass-asset-host

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Feb 2, 2022

Why: Icon sprites consumed via SVG <use> cannot reference cross-domain resources, so it must be served from the same host.

Related Slack discussion: https://gsa-tts.slack.com/archives/C0NGESUN5/p1643749421139809

Comment on lines +269 to +270
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comes from the fallback here:

(IdentityConfig.store.asset_host.presence || IdentityConfig.store.domain_name) if request

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this branch is not covered by tests, I'd consider adding it

if we don't want to stub Rails.env, we could do some dependency injection:

def asset_host(rails_env = Rails.env)
  if rails_env.production?

and then in the test pass in a ActiveSupport::EnvironmentInquirer.new("production") or ActiveSupport::EnvironmentInquirer.new("development") etc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this branch is not covered by tests, I'd consider adding it

👍 Added specs in rebased 8f82e3b.

Went for stubbing due to light preference to keep asset_host a private method.

Comment on lines +271 to +272
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aduth
Copy link
Contributor Author

aduth commented Feb 2, 2022

Confirmed that this works in my personal sandbox, which otherwise uses a CDN for assets:

image

**Why**: Icon sprites consumed via SVG `<use>` cannot reference cross-domain resources, so it must be served from the same host.
@aduth aduth force-pushed the aduth-icon-bypass-asset-host branch from a28fe8f to 8f82e3b Compare February 2, 2022 13:28
@aduth aduth changed the base branch from aduth-process-list to main February 2, 2022 13:28
@aduth aduth marked this pull request as ready for review February 2, 2022 13:28
@aduth aduth merged commit d349b9e into main Feb 2, 2022
@aduth aduth deleted the aduth-icon-bypass-asset-host branch February 2, 2022 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants