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

fix: witness not provided for fallback on routes with an alias #3428

Conversation

sesi200
Copy link
Contributor

@sesi200 sesi200 commented Oct 31, 2023

Description

The asset canister does not provide all required absence proofs when the file is not found if the folders are nested deep enough with a few present files. This PR adds the last corner case that wasn't covered before and (at least in my eyes) simplifies proof generation.

Also see original bug report below

Fixes SDK-1298

How Has This Been Tested?

Added a file to the existing test case which matches the minimal repro from the bug report. Compared against master and there the test does not pass with the same changes.

Checklist:

  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have updated asset.wasm

Original bug report:

The asset canister is not providing a witness for fallback responses on routes with an alias.

For example:

If the /hello.html path exists in the asset canister, then making a request against /hello/world will return a fallback to /index.html as expected.

However, if the /hello/index.html path exists in the asset canister then making a request against /hello/world will not work.

This is an example tree returned in this scenario:

55799(
	[1, 
		[4, h'ADE6FBA949CFF9CCE12F3BA7C5C43A99D4166A2C44C477DBA04FD9DD19B579C1'], 
		[2, h'687474705F65787072',  // http_expr
			[1, 
				[1, 
					[1, 
						[2, h'', 
							[2, h'3C243E', // <$>
								[4, h'1437EE3D48A40762C8FDF72EE779466EF67F0D8BA1CA1DF40AA467467621A036']
							]
						],
						[1, 
							[2, h'3C2A3E', // <$>
								[1, 
									[2, h'162BD2F8BA53318BDF0289F9B70393E98E51DF0A3EE6DD79F53399DFE7888ED7', 
										[2, h'', 
											[2, h'49E9CF44E617D88CA88D293FC481E27CBD342D84DC41A3BFFC56999A30EA3626', 
												[3, h'']
											]
										]
									],
									[2, h'C504735B4B6812FD0292109ADCF7C5757C3F34BE28A9BB6DF3F9A55BACCE112C',
										[2, h'',
											[2, h'06C44C86B219D1FDDAE568187643D3954AD683AFF6774CC9E9525244325BA757',
												[3, h'']
											]
										]
									]
								]
							], 
							[4, h'B36CBF7EBF56FDA84A6D1C28F77FE0B5DDC04BE75E0E41546F2516FD5B4A51BF']
						]
					],
					[1, 
						[4, h'255C240DFCFA0F9A2FF734ACFE78CA9A4F03A2CBEAFFAF0C489CAB9AE738AAA4'],
						[1, 
							[1, 
								[2, h'68656C6C6F', // hello
									[1, 
										[4, h'DAB792DA7FEF5FEDE38CB0DA667ED0263A2B9E0F8AF18ABD1BA0BCE344452379'], 
										[1, 
											[2, h'3C243E', // <$>
												[4, h'70BD8799C26666D3A3F66BB2EAE6A1966CB71E7C25E324B76244BC6565A67F43']
											],
											[1, 
												[2, h'696E646578', // index
													[4, h'1EA146409F4F856B5554A0240E1C68A303CA9E31D77A7106A7D89D5C54827361']
												],
												[2, h'696E6465782E68746D6C',  // index.html
													[4, h'1EA146409F4F856B5554A0240E1C68A303CA9E31D77A7106A7D89D5C54827361']
												]
											]
										]
									]
								], 
								[4, h'051F604A8D380B764638B19563B715A5FE71570B0EE9E892D229ED5B5C4D4157']
							],
							[4, h'7000A190977ABC4E6F5A5E810B4A1A0F3CAC15B393E6B592DE810B4A4DCE5209']
						]
					]
				], 
				[4, h'47AC2ABC8EE67BE2D6ED51A94D23E99100E18FD2E0005833F319BB93B7270F5E']
			]
		]
	]
)

Response verification is failing on this tree because the lookup operation is returning Unknown. The lookup path that returns Unknown is [687474705F65787072, 68656C6C6F, , 3C2A3E] (this translates to ["http_expr", "hello", ""]).  Following that path in the tree above reaches the Pruned node with DAB792DA7FEF5FEDE38CB0DA667ED0263A2B9E0F8AF18ABD1BA0BCE344452379 as it's label. Since empty string is lower than DAB79... it will return absent.

If the asset canister provides a witness for ["http_expr", "hello", ""] then this should fix the issue.

@sesi200 sesi200 marked this pull request as ready for review October 31, 2023 15:51
@sesi200 sesi200 requested review from chenyan-dfinity and a team as code owners October 31, 2023 15:52
CHANGELOG.md Outdated Show resolved Hide resolved
@@ -84,7 +84,7 @@ pub fn verify_response(

fn certified_http_request(state: &State, request: HttpRequest) -> HttpResponse {
let response = state.http_request(request.clone(), &[], unused_callback());
assert!(verify_response(state, &request, &response).expect("Certificate validation failed."));
let Ok(_) = verify_response(state, &request, &response) else {panic!("Response verification failed: {:#?}", response)};

Choose a reason for hiding this comment

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

This doesn't fail the test if verify_response() returns false.

I applied the change on line 512 to master and the test failed, as expected
I applied this change as well and the test passed, when it should have failed.

Choose a reason for hiding this comment

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

Also I think it would be helpful to display (panic with) the ResponseVerificationError (inside the Err) as well as the response

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that! New version reports all available information

@sesi200 sesi200 merged commit 5f25685 into master Nov 2, 2023
168 checks passed
@sesi200 sesi200 deleted the SDK-1298-witness-not-provided-for-fallback-on-routes-with-an-alias branch November 2, 2023 07:15
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