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

Quarkus CLI finds and uses rogue application.properties files and crashes #19549

Closed
jamesfalkner opened this issue Aug 20, 2021 · 14 comments · Fixed by #20385 or #22331
Closed

Quarkus CLI finds and uses rogue application.properties files and crashes #19549

jamesfalkner opened this issue Aug 20, 2021 · 14 comments · Fixed by #20385 or #22331
Assignees
Labels
area/cli Related to quarkus cli (not maven/gradle/etc.) area/config area/jbang Issues related to when using jbang.dev with Quarkus kind/bug Something isn't working
Milestone

Comments

@jamesfalkner
Copy link

jamesfalkner commented Aug 20, 2021

Describe the bug

I recently installed the Quarkus CLI and upon first execution of quarkus version it spit out tons of warnings and I had no idea why (I was not in any project directory, rather was in my home directory where I never put projects). Turns out I had a rogue application.properties in the directory (with no other project-related files) and it loaded and parsed it as part of the CLI runtime.

If that file is a binary (e.g. by renaming a .zip file to application.properties) then Quarkus CLI crashes.

Expected behavior

No warnings on first execution, in a directory with no project.

Actual behavior

With a valid properties file:

~ quarkus version
2021-08-20 12:48:03,173 WARN  [io.qua.config] (main) Unrecognized configuration key "quarkus.hibernate-orm.sql-load-script" was provided; it will be ignored; verify that the dependency extension for this configuration is set or that you did not make a typo
2021-08-20 12:48:03,173 WARN  [io.qua.config] (main) Unrecognized configuration key "quarkus.datasource.username" was provided; it will be ignored; verify that the dependency extension for this configuration is set or that you did not make a typo
2021-08-20 12:48:03,173 WARN  [io.qua.config] (main) Unrecognized configuration key "quarkus.datasource.jdbc.url" was provided; it will be ignored; verify that the dependency extension for this configuration is set or that you did not make a typo
2021-08-20 12:48:03,174 WARN  [io.qua.config] (main) Unrecognized configuration key "quarkus.hibernate-orm.database.generation" was provided; it will be ignored; verify that the dependency extension for this configuration is set or that you did not make a typo
2021-08-20 12:48:03,174 WARN  [io.qua.config] (main) Unrecognized configuration key "quarkus.datasource.db-kind" was provided; it will be ignored; verify that the dependency extension for this configuration is set or that you did not make a typo
2021-08-20 12:48:03,174 WARN  [io.qua.config] (main) Unrecognized configuration key "quarkus.datasource.password" was provided; it will be ignored; verify that the dependency extension for this configuration is set or that you did not make a typo
Client Version 2.1.2.Final

With a binary named application.properties:

~ cp v3.11.0.zip application.properties
➜  ~ quarkus version
Exception in thread "main" java.lang.ExceptionInInitializerError
	at io.quarkus.runner.ApplicationImpl.<clinit>(ApplicationImpl.zig:60)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:490)
	at java.base/java.lang.Class.newInstance(Class.java:584)
	at io.quarkus.runtime.Quarkus.run(Quarkus.java:65)
	at io.quarkus.runtime.Quarkus.run(Quarkus.java:42)
	at io.quarkus.runner.GeneratedMain.main(GeneratedMain.zig:30)
Caused by: java.lang.IllegalArgumentException: Malformed \uxxxx encoding.

How to Reproduce?

  1. Take any file and call it application.properties
  2. Install CLI with: curl -Ls https://sh.jbang.dev | bash -s - app install --fresh --force quarkus@quarkusio
  3. While in the same directory as the rogue file, run quarkus version

Output of uname -a or ver

Darwin jfalkner-OSX 20.5.0 Darwin Kernel Version 20.5.0: Sat May 8 05:10:33 PDT 2021; root:xnu-7195.121.3~9/RELEASE_X86_64 x86_64

Output of java -version

openjdk version "11.0.10" 2021-01-19
OpenJDK Runtime Environment AdoptOpenJDK (build 11.0.10+9)
OpenJDK 64-Bit Server VM AdoptOpenJDK (build 11.0.10+9, mixed mode)

GraalVM version (if different from Java)

N/A

Quarkus version or git rev

Client Version 2.1.2.Final

Build tool (ie. output of mvnw --version or gradlew --version)

Quarkus CLI

Additional information

I'm concerned it may be possible to do something nasty with a cleverly-designed application.properties.

@jamesfalkner jamesfalkner added the kind/bug Something isn't working label Aug 20, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Aug 20, 2021

/cc @ebullient, @maxandersen, @quarkusio/devtools

@quarkus-bot quarkus-bot bot added area/cli Related to quarkus cli (not maven/gradle/etc.) area/jbang Issues related to when using jbang.dev with Quarkus labels Aug 20, 2021
@ebullient ebullient self-assigned this Aug 20, 2021
@maxandersen
Copy link
Member

we'll have to mock around with the config sources and their levels.

No idea if feasible but if we could just have it ignore application.properties and just use quarkus-cli.properties (or similar) instead and there wouldn't be conflicts.

@radcortez
Copy link
Member

Ideally, we need to skip the registration of such sources. We could probably find some clever way to ignore the values coming from the sources, but the case where you have a non-readable file will always crash the application.

I believe we have two options:

  1. Have some kind of way for us to recognize that we are running a Quarkus CLI, and so when we are generating the Config, we can pick which sources to use (or not)

  2. Provide some kind of hook, where a Quarkus application may influence how the Config is going to be initialized. This provides some more flexibility, but probably a little more dangerous. We can probably don't expose this directly to the user and make it a Quarkus internal thing only.

Also, what about dependencies that may have required configuration in application.properties or other files? Even the CLI module has an application.properties file :)

@ebullient
Copy link
Member

ebullient commented Sep 20, 2021

+100 on skipping registration. That is by far the cleanest.

build time reading of application.properties is fine. It is specifically runtime processing of said sources that should be disable-able. So if that means that there is the ability to (later in the build) remove those config sources (to avoid a chicken/egg problem), that would work.

@radcortez
Copy link
Member

From what I can tell, on the CLI code, we don't have many options to influence how Quarkus is going to behave, because it just calls Quarkus.run and that is it. Are there any build steps that are CLI-specific only that we can use?

I guess that one easy option is to add a build-time configuration property that can be used by the CLI module and allows you to control how these sources are registered.

@radcortez
Copy link
Member

Not completely fixed yet. #20385 add a way to be able to fix it.

@ebullient will take care of the rest :)

@maxandersen
Copy link
Member

looking at #20385 why is this connected to picocli in any shape or form ? @ebullient @radcortez ?

One should be able to configure/set this up for any kind of Quarkus no matter if you use picocli or not ...maybe the code can do that but just seeing that the commit adds config build steps to picocli extension which to me sounds very wrong?

@ebullient
Copy link
Member

If you feel strongly that it applies more generally, fine, but IMO, the picocli extension is the most likely to need this out of the gate (where it might not be as common elsewhere).

I don't see a problem with it (nor do I see it as very wrong), but I don't have a problem with it being generalized either.

@maxandersen
Copy link
Member

Picocli is not necessary to use @quarkusmain so seems like it should be possible without being tied to picocli imo.

@radcortez
Copy link
Member

We discussed in the PR to provide a way to register the specific builders to customize how the Config is used. I can add the support for it in a separate PR.

@gsmet
Copy link
Member

gsmet commented Dec 7, 2021

Where are we on this? It's marked for 2.6 but I don't see it fully fixed, right?

@gsmet gsmet removed this from the 2.6 - main milestone Dec 7, 2021
@radcortez
Copy link
Member

Where are we on this? It's marked for 2.6 but I don't see it fully fixed, right?

No, I thought @ebullient was going to do it. @ebullient do you want me to do it?

@ebullient
Copy link
Member

I have hands/brain tied up.. if you can, yes!

@radcortez
Copy link
Member

No worries, I'll pick this up.

@radcortez radcortez assigned radcortez and unassigned ebullient Dec 8, 2021
Repository owner moved this from In Progress to Done in Quarkus Roadmap/Planning Jan 6, 2022
@quarkus-bot quarkus-bot bot added this to the 2.7 - main milestone Jan 6, 2022
@gsmet gsmet modified the milestones: 2.7 - main, 2.6.2.Final Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli Related to quarkus cli (not maven/gradle/etc.) area/config area/jbang Issues related to when using jbang.dev with Quarkus kind/bug Something isn't working
Projects
Status: Done
5 participants