Skip to content

Add pki-https feature to prevent local cert validation on all-features#1091

Closed
benalleng wants to merge 1 commit intopayjoin:masterfrom
benalleng:payjoin-cli-pki-feature
Closed

Add pki-https feature to prevent local cert validation on all-features#1091
benalleng wants to merge 1 commit intopayjoin:masterfrom
benalleng:payjoin-cli-pki-feature

Conversation

@benalleng
Copy link
Copy Markdown
Collaborator

@benalleng benalleng commented Sep 18, 2025

By adding a pki-https flag and creating specific cfg flags through the cli crate we can effectively prevent local cert validation from occurring when building the crate with all features.

I think perhaps some more clarification is needed as to how we want to handle tests in these cases.

Closes #451

Chat gpt helped me grok once and for all how cfg flags work

Pull Request Checklist

Please confirm the following before requesting review:

set -e

cargo test --locked --package payjoin-cli --verbose --all-features
cargo test --locked --package payjoin-cli --verbose --no-default-features --features "v1,v2,_manual-tls"
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The tests fail with the previous setup where all features obviously prevents these tests from working as expected

for feature in "${features[@]}"; do
# Don't duplicate --all-targets clippy. Clilppy end-user code, not tests.
cargo clippy --no-default-features --features "$feature" -- -D warnings
cargo clippy --no-default-features --features "$feature,_manual-tls" -- -D warnings
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

There is a holdout with the certificate_key in the config being dead code when building with just v1 or v2 enabled.

@arminsabouri
Copy link
Copy Markdown
Collaborator

@benalleng
Couple typos in the commit message ;)

I think perhaps some more clarification is needed as to how wew wwant to
handle tests in these cases.

@benalleng
Copy link
Copy Markdown
Collaborator Author

Couple typos in the commit message ;)

Rip, my keyboard's w key is broken and either gets 0 presses or 2+, will amend, but this will likely happen again 😢

@coveralls
Copy link
Copy Markdown
Collaborator

coveralls commented Sep 18, 2025

Pull Request Test Coverage Report for Build 17841014742

Details

  • 3 of 4 (75.0%) changed or added relevant lines in 1 file are covered.
  • 539 unchanged lines in 9 files lost coverage.
  • Overall coverage decreased (-5.6%) to 79.013%

Changes Missing Coverage Covered Lines Changed/Added Lines %
payjoin-cli/src/app/mod.rs 3 4 75.0%
Files with Coverage Reduction New Missed Lines %
payjoin-cli/src/main.rs 3 65.22%
payjoin/src/core/receive/v2/mod.rs 3 92.03%
payjoin/src/core/send/v2/session.rs 4 94.12%
payjoin/src/core/send/v1.rs 13 88.53%
payjoin/src/core/send/v2/mod.rs 17 85.99%
payjoin-cli/src/app/v2/ohttp.rs 42 0.0%
payjoin-cli/src/app/config.rs 47 53.18%
payjoin-cli/src/db/v2.rs 140 0.0%
payjoin-cli/src/app/v2/mod.rs 270 0.0%
Totals Coverage Status
Change from base Build 17837477968: -5.6%
Covered Lines: 7556
Relevant Lines: 9563

💛 - Coveralls

@benalleng benalleng force-pushed the payjoin-cli-pki-feature branch from 88f4424 to afe69a9 Compare September 18, 2025 20:14
@spacebear21
Copy link
Copy Markdown
Collaborator

Code coverage is down 5.5% which probably means we're skipping over some tests here.

@benalleng benalleng force-pushed the payjoin-cli-pki-feature branch 2 times, most recently from ef714af to f1be0cf Compare September 18, 2025 20:41
@benalleng
Copy link
Copy Markdown
Collaborator Author

I am a little confused about what is being skipped here, based on this it seems like every test in the e2e-cli is still run

By adding a pki-https flag and creating specific cfg flags through the
cli crate we can effectively prevent local cert validation from
occurring when building the crate with all features.

I think perhaps some more clarification is needed as to how we want to
handle tests in these cases.
@benalleng benalleng force-pushed the payjoin-cli-pki-feature branch from f1be0cf to 7d8163a Compare September 18, 2025 20:48
@nothingmuch
Copy link
Copy Markdown
Contributor

hmm, isn't _manual_tls fine now?

we no longer skip certificate verification anywhere, instead we just allow a root certificate to be added manually, which is a useful feature in its own right and not necessarily insecure

unless i'm missing something i think the issue can be closed without merging this and we can just rename and document the _manual_tls flag accordingly

@benalleng
Copy link
Copy Markdown
Collaborator Author

Ok, I wasn't sure if we wanted to go a step further and prevent these manual root certificate functions accessible at all when building with --all-features I guess this got completed before the _manual-tls name change when we changed how we handled root certs way back.

@nothingmuch
Copy link
Copy Markdown
Contributor

Yeah it happened in several steps, I guess technically #913 resolved it, but i forgot to note that since the intent was to fix a testing problem

@benalleng benalleng closed this Sep 19, 2025
@benalleng benalleng deleted the payjoin-cli-pki-feature branch October 6, 2025 18:40
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.

having _danger-local-https may result in insecure payjoin-cli build if built with --all-features

5 participants