Skip to content

feat: implement ohttp gateway middleware#1306

Open
Harshdev098 wants to merge 3 commits intopayjoin:masterfrom
Harshdev098:ohttpGateway-middleware
Open

feat: implement ohttp gateway middleware#1306
Harshdev098 wants to merge 3 commits intopayjoin:masterfrom
Harshdev098:ohttpGateway-middleware

Conversation

@Harshdev098
Copy link
Contributor

@Harshdev098 Harshdev098 commented Feb 1, 2026 •
edited
Loading

part of #941

added ohttp gateway middleware in payjoin-services.

have used claude to fix errors in the development works

Pull Request Checklist

Please confirm the following before requesting review:

@zealsham
Copy link
Collaborator

zealsham commented Feb 3, 2026

ThankS for working on this @Harshdev098. Ideally we would want this middleware to be written with axum so it would fit in with the rest of the middlwares we plan to add, @spacebear21 this is correct right ?

@spacebear21
Copy link
Collaborator

spacebear21 commented Feb 3, 2026

we would want this middleware to be written with axum so it would fit in with the rest of the middlwares we plan to add

Yes exactly. The gateway should be an axum middleware written mostly from scratch similar to @zealsham's ongoing work in #1296. Duplication could also be reduced by first making the helper utils (encapsulate_xxx, decapsulate_xxx, constants...) pub in the ohttp-relay crate and reusing them in payjoin-service.

@Harshdev098
Copy link
Contributor Author

Harshdev098 commented Feb 3, 2026

okay will use axum in the middleware and update you!

@Harshdev098 Harshdev098 force-pushed the ohttpGateway-middleware branch from e454ea4 to f8fcdfe Compare February 6, 2026 09:25
@Harshdev098
Copy link
Contributor Author

Harshdev098 commented Feb 6, 2026

@spacebear21 Can you please review it, have updated the PR to use axum, also should I refactor the payjoin-directory becuase in the middleware I have done encapsulation and decapsulation, and the payjoin-directory is duplicating it which causes the failures of integration tests.

@zealsham
Copy link
Collaborator

zealsham commented Feb 6, 2026

@Harshdev098 I still see so many traces of hyper in the recent commit. can you take a look at #1101, should help with re-writting the ohttp middleware in axum.

@spacebear21
Copy link
Collaborator

spacebear21 commented Feb 6, 2026 •
edited
Loading

There are also many comments deleted with no explanation, which seems like unreviewed (and undisclosed) LLM work.

@Harshdev098 Harshdev098 force-pushed the ohttpGateway-middleware branch 4 times, most recently from 156c799 to 45ac88d Compare February 7, 2026 09:33
@Harshdev098 Harshdev098 force-pushed the ohttpGateway-middleware branch from 45ac88d to 43b12a9 Compare February 7, 2026 09:36
@Harshdev098
Copy link
Contributor Author

Harshdev098 commented Feb 7, 2026

@zealsham @spacebear21 Have updated the code as you referenced and disclosed my claude usage too in the description!

@zealsham
Copy link
Collaborator

zealsham commented Feb 7, 2026

Haven’t taken good look since I’m away from keyboard , However you need the test cases passing, it’s the only way we can tell everything works well.

I see your prior question about removing directory code because decapsulation is done twice. The answer to that is you have to do what you think we make the test cases pass. If you think doing something will complete the Pr, then do it. Reviewers are here to catch something totally off.

Remember a complete Pr ready for review is the Pr with all green checks in CI

cheers!

@coveralls
Copy link
Collaborator

coveralls commented Feb 8, 2026 •
edited
Loading

Pull Request Test Coverage Report for Build 22011074362

Details

  • 178 of 239 (74.48%) changed or added relevant lines in 4 files are covered.
  • 16 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.3%) to 82.985%
Changes Missing Coverage Covered Lines Changed/Added Lines %
payjoin-directory/src/lib.rs 27 37 72.97%
payjoin-service/src/lib.rs 30 41 73.17%
ohttp-relay/src/gateway_helpers.rs 49 69 71.01%
payjoin-service/src/ohttp/middleware.rs 72 92 78.26%
Files with Coverage Reduction New Missed Lines %
payjoin-directory/src/lib.rs 16 42.55%
Totals Coverage Status
Change from base Build 21997882549: -0.3%
Covered Lines: 10286
Relevant Lines: 12395
💛 - Coveralls

@Harshdev098 Harshdev098 force-pushed the ohttpGateway-middleware branch from 89d951a to 2a0def2 Compare February 8, 2026 13:43
@Harshdev098
Copy link
Contributor Author

Harshdev098 commented Feb 8, 2026

Hey @zealsham @spacebear21 Can you please review it! have removed the ohttp handling from the directory in order to pass the test and prevent double encapsulation and decapsulation

@Harshdev098 Harshdev098 force-pushed the ohttpGateway-middleware branch from 2a0def2 to a928973 Compare February 8, 2026 13:55
@zealsham
Copy link
Collaborator

zealsham commented Feb 8, 2026

Hey @zealsham @spacebear21 Can you please review it! have removed the ohttp handling from the directory in order to pass the test and prevent double encapsulation and decapsulation

The tests does not pass ? , can you see the CI result for your pull request ?.

@Harshdev098
Copy link
Contributor Author

Harshdev098 commented Feb 8, 2026

Actually I think I have removed/added some dependencies earlier in the commits and forgot to run update lock file script

@Harshdev098 Harshdev098 force-pushed the ohttpGateway-middleware branch from a928973 to 5957c0a Compare February 8, 2026 17:54
@Harshdev098
Copy link
Contributor Author

Harshdev098 commented Feb 8, 2026

@zealsham the tests are passing now!

zealsham
zealsham suggested changes Feb 13, 2026
View reviewed changes
Copy link
Collaborator

@zealsham zealsham left a comment

Choose a reason for hiding this comment

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

Cack!, jsut some review changes and an extra review eyes from one other person to get this in

payjoin-service/src/ohttp/middleware.rs
Comment on lines 51 to 54
Ok(result) => result,
Err(e) => {
debug!("OHTTP decapsulation failed: {}", e);
return match e {
Copy link
Collaborator

@zealsham zealsham Feb 13, 2026

Choose a reason for hiding this comment

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

This should be an error!, rather than a debug

payjoin-service/src/ohttp/middleware.rs Outdated
Comment on lines 68 to 69
debug!("Invalid URI in BHTTP: {}", e);
return (StatusCode::BAD_REQUEST, "Invalid URI").into_response();
Copy link
Collaborator

@zealsham zealsham Feb 13, 2026

Choose a reason for hiding this comment

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

should be an error

payjoin-service/src/ohttp/middleware.rs
Comment on lines 112 to 115
Err(e) => {
debug!("OHTTP encapsulation failed: {}", e);
return (StatusCode::INTERNAL_SERVER_ERROR, "Failed to encapsulate response")
.into_response();
Copy link
Collaborator

@zealsham zealsham Feb 13, 2026

Choose a reason for hiding this comment

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

same as above , should be an error!

payjoin-service/src/ohttp/middleware.rs Outdated
let server = create_test_ohttp_server();
let sentinel_tag = SentinelTag::new([0u8; 32]);

let config1 = OhttpGatewayConfig::new(server, sentinel_tag);
Copy link
Collaborator

@zealsham zealsham Feb 13, 2026

Choose a reason for hiding this comment

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

What is the value of this test , the test_config_creation test seems to cover this test. why are we testing that this is cloneable

Copy link
Collaborator

@zealsham zealsham Feb 13, 2026

Choose a reason for hiding this comment

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

Also i don't think checking if the memory size is greater than 0 is a good way to validate config. it could be all junk data and still greater than 0

payjoin-service/src/ohttp/middleware.rs
Comment on lines 125 to 129
(
StatusCode::BAD_REQUEST,
[("content-type", "application/problem+json")],
OHTTP_KEY_REJECTION_JSON,
)
Copy link
Collaborator

@zealsham zealsham Feb 13, 2026

Choose a reason for hiding this comment

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

nit: use https://docs.rs/http/latest/http/header/constant.CONTENT_TYPE.html

ohttp-relay/src/gateway_helpers.rs
pub fn decapsulate_ohttp_request(
ohttp_body: &[u8],
ohttp_server: &ohttp::Server,
) -> Result<(DecapsulatedRequest, ohttp::ServerResponse), GatewayError> {
Copy link
Collaborator

@zealsham zealsham Feb 13, 2026

Choose a reason for hiding this comment

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

Had my reservations about the error variant returned but then again the primary reason decapsulation fails most times its due to invalid keys , so maybe this can stay this way

@Harshdev098 Harshdev098 force-pushed the ohttpGateway-middleware branch from 5957c0a to 8901bec Compare February 14, 2026 04:22
@Harshdev098
Copy link
Contributor Author

Harshdev098 commented Feb 14, 2026

@zealsham @spacebear21 Have updated the pr with required changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@zealsham zealsham zealsham requested changes

Reviewers whose approvals may not affect merge requirements

At least 0 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Harshdev098 @zealsham @spacebear21 @coveralls