Repository navigation
Block *.js.map files in production. - #48301
Conversation
*.js.map files will still be allowed in dev mode for debugging purposes. Fixes keycloak#47545 Signed-off-by: Stan Silvert <ssilvert@redhat.com>
|
Wouldn't it be easier to "just" disable the generation of source maps for a build? |
I don't want to get rid of source maps altogether. They can be useful. |
|
The only reason we added them in the first place was so that we could have stack traces in our errors, but most of the time people send us a screenshot of the error page instead. If we don't have them in production we don't need them at all IMO |
There was a problem hiding this comment.
I still think this is stupid, but I'm fine with keeping them for the one in a million, where it could be useful
|
I do want to say that this PR also protects customers against inadvertently exposing their own map files, I also think the whole thing is a little stupid as this really doesn't provide much real security but it makes the auditing tools happy. |
There was a problem hiding this comment.
I haven't looked into how these .js.map gets included as static resources or how they are served as I am in a hurry. I just commented on the current code. Whether this is desired solution for #47545 I'll let someone else decide and approve.
| void filterSourceMapRequests(BuildProducer<FilterBuildItem> filters, KeycloakRecorder recorder) { | ||
| var filter = recorder.getRejectSourceMapFilter(); | ||
| if (filter != null) { | ||
| filters.produce(new FilterBuildItem(filter, SecurityHandlerPriorities.CORS + 1)); |
There was a problem hiding this comment.
In general, IMO you should decide if it is a DEV Mode in a build step, not in the recorder where the byte code is already recorded.
So I suggest to try following:
@BuildStep(>- drop
@Record(ExecutionTime.STATIC_INIT)andKeycloakRecorder recorderand@Consume(ConfigBuildItem.class) - use
filters.produce(new FilterBuildItem(new RejectSourceMapFilter(), SecurityHandlerPriorities.CORS + 1)), it should work as there is no state and it has (implicit) no arg ctor
There was a problem hiding this comment.
@michalvavrik I'm a little out of my element here. Claude Code tells me:
This is a good idea in general Quarkus projects, but it won't work for Keycloak. IsDevelopment.class checks Quarkus's LaunchMode, which is always NORMAL in Keycloak — even when running start-dev. That's exactly why we ended up using Environment.isDevMode() in the recorder rather than LaunchMode.
Does this make sense?
There was a problem hiding this comment.
Unfortunately it does. Our start-dev != quarkus dev. At build time we will either be in our dev or prod mode. Much like org.keycloak.quarkus.deployment.IsReAgumentation, you could introduce a IsKeycloakDevMode based upon the Environment call.
There was a problem hiding this comment.
@ssilvert I didn't know about what @shawkins says, but my suggestion was about keeping it in the build step, I don't care how you detect it in the build step. IMO you can use org.keycloak.quarkus.runtime.Environment#isDevMode inside of the build step or introduce a BooleanSupplier, whatever works for you.
Just to be clear, I am suggesting it because I think it is a better way, practically it makes negligible difference :-) and you don't need to do it.
Signed-off-by: Stan Silvert <ssilvert@redhat.com>
Signed-off-by: Stan Silvert <ssilvert@redhat.com>
Signed-off-by: Stan Silvert <ssilvert@redhat.com>
* Block *.js.map files in production. *.js.map files will still be allowed in dev mode for debugging purposes. Fixes keycloak#47545 Signed-off-by: Stan Silvert <ssilvert@redhat.com> * Minor changes from PR review. Signed-off-by: Stan Silvert <ssilvert@redhat.com> * Conditionally install the filter at build time. Signed-off-by: Stan Silvert <ssilvert@redhat.com> * Restore openapi.yaml, which was changed accidentally. Signed-off-by: Stan Silvert <ssilvert@redhat.com> --------- Signed-off-by: Stan Silvert <ssilvert@redhat.com>
*.js.map files will still be allowed in dev mode for debugging purposes.
Fixes #47545
I tried to find a clean way to write a test for this, but a true test requires running in production mode. This is possible with the existing test infrastructure using --db=dev-file, but no existing test combines start mode with HTTP assertions, making it non-trivial.
An alternative would be adding a config flag to enable blocking in dev mode, similar to --http-accept-non-normalized-paths. However, this tests the flag mechanism rather than the actual production behavior, and I don't want to add a command line argument solely for testability.