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

additionalHeaders not working as expected #37

Closed
JuBan1 opened this issue Nov 1, 2023 · 18 comments · Fixed by #146
Closed

additionalHeaders not working as expected #37

JuBan1 opened this issue Nov 1, 2023 · 18 comments · Fixed by #146
Assignees
Labels
enhancement New feature or request

Comments

@JuBan1
Copy link

JuBan1 commented Nov 1, 2023

The doc of rememberWebViewState states:

param additionalHttpHeaders Optional, additional HTTP headers that are passed to [AccompanistWebView.loadUrl]. Note that these headers are used for all subsequent requests of the WebView.

It seems the bolded part isn't actually true. The headers are passed along via the initial WebContent.Url object, but discarded when navigating inside the webview or using navigator.loadUrl().

I tried it by attaching an Authorization header to a request; the initial request worked, subsequent requests were rejected.

I can see a few options here:

  1. Change the docs and use the headers only for the initial request. But that would leave us with no option to actually provide universal headers.
  2. Keep the headers and apply them to every subsequent navigator.loadUrl() call. That would apply the headers to all requests created via the navigator, but not to requests created by the webview itself.
  3. Apply these headers to all requests. That's probably a bit more work, on Android it would involve overrideUrlLoading, on Cef I don't know.

My personal, related issue: I want to attach an Authorization header to all requests (if possible, only requests to a certain domain). I think at the moment this is impossible.


Tested using 1.6.0 and Android, but I think the current master works the same (except that additionalHeaders don't work for desktop at all in 1.6.0).

Thanks for stepping up to build and maintain an KMP webview! 💪

@KevinnZou
Copy link
Owner

@JuBan1 Thank you for your suggestions! After checking, I also confirmed that additionalHttpHeaders only works for the initial request. I apologize for any confusion caused by the comment, which was copied from the accompanist Webview. I will make sure to address this issue in the future.

However, I am currently focused on resolving the JsBridge and other issues, so it may take some time before I can start working on this particular problem. If you are interested, you are welcome to submit a PR for the Android and iOS implementation. For the Desktop side, I think @DatL4g would be happy to provide assistance.

@KevinnZou KevinnZou self-assigned this Nov 2, 2023
@KevinnZou KevinnZou added the enhancement New feature or request label Nov 2, 2023
@JuBan1
Copy link
Author

JuBan1 commented Nov 3, 2023

I think a good first step would be to figure out what the goal should be.

(1) and (2) are easy to implement, but are lacking.

For my purposes I would need at least (3), but after thinking about it a little it might be a better idea to basically implement shouldOverrideUrlLoading. That allows users to intelligently add headers to all requests but also do things like intercept certain URLs or prevent certain domains.

I'm thinking users can add an RequestInterceptor to WebViewNavigator; the interceptor is a callback which is triggered for every (GET) request and enables you do (a) allow it, (b) modify it or (c) block it. I think this is possible on Android via shouldOverrideUrlLoading, on Cef via onBeforeBrowse, and I guess it kinda works on iOS too.

@KevinnZou
Copy link
Owner

I agree with you. I think RequestInterceptor is a great idea and I would go with this plan.

@KevinnZou
Copy link
Owner

KevinnZou commented Nov 29, 2023

@JuBan1 Hi, I have implemented the basic feature for RequestInterceptor on this branch. It should work well on Android and iOS, but have some problem on Desktop. Can you review it and give some suggestions? Thank you so much!

@KevinnZou
Copy link
Owner

@DatL4g Could you also have a look at the Desktop implementation on this branch? I am trying to intercept the request in CefRequestHandler.onBeforeBrowse. It did intercept the request but will also block future requests if it return false and reload another Url like I did in the example.

@DatL4g
Copy link
Collaborator

DatL4g commented Nov 29, 2023

Adding a CefRequestHandler means overriding the default CefRequestHandler.

We had that problem before (user-agent) and we (or I) have to reimplement the default behavior in and make setting a CefRequestHandler extend the default reimplementation instead of overriding.

Additionally this doesn't only break the ``onBeforeBrowse` implementation, it breaks all request handling methods.

I don't have that much time currently as I'm in a release phase right now, best would be to add it directly to KCEF so we don't have to deal with it in this library, maybe you could add a PR if you have time or we have to wait until I have time again (could be anytime in December).

@JuBan1
Copy link
Author

JuBan1 commented Nov 29, 2023

Hey, thanks for tackling this @KevinnZou! I have a half-finished implementation as well, and I got stuck on the desktop implementation too. So that strengthens @DatL4g's point of moving it into KCEF if possible. Unfortunately I think I'm a little out of my depth there :)

I pushed my implementation into a PR for inspiration. (Warning: nothing is finished in there!)

I have had more luck with overriding onBeforeResourceLoad and discovered that modifying CefRequests works fine. But cancelling a request breaks the implementation. So it's probably a dead end too.

Besides, I've discovered a few more problems:

  • My initial intent was to be able to set the Authorization-header for particular websites. This works on Android, but CEF actually filters out the Authorization header (and probably a few others) if you try to set it via setHeaderMap(). So this isn't a multiplatform solution, and authorization management would have to be a separate feature...
  • Different platforms have very different scenarios in which they trigger the request interception. On Android, shouldOverrideUrlLoading is only called for a few URLs but on CEF onBeforeResourceLoad is called for literally everything: images, css-files, etc. On iOS decidePolicyForNavigationAction probably has its own rules. So it's tough to find common grounds so it works well on every platform without huge differences in behavior. (For example, in my implementation I tried to exclude non-mainframe requests for CEF)

The only thing I am happy about is the RequestInterceptor and how it is used. I think it is a good interface for users to work with.

On a side note, (K)CEF provides a CEFRequestHandlerAdapter that has default-implementations for all methods, meaning you don't have to do that yourself

@KevinnZou
Copy link
Owner

@JuBan1 Thank you for your great work! I really appreciate your idea for RequestResult.Modify, it eliminates the need for users to manually reload the URL. I will incorporate your ideas into my implementation, as they are extremely helpful to me.

Regarding the issues you mentioned, I will collaborate with @DatL4g to see whether the Authorization header problem can be solved. Additionally, I also observed that shouldOverrideUrlLoading does not intercept all URLs like onBeforeBrowse does. I will also work on finding a solution for this issue.

Generally, I think we should be able to support Android and iOS first. As for the Desktop side, I will attempt to address the CefRequestHandler issue following @DatL4g's instructions. If I am unable to do that, we will have to wait for @DatL4g to resolve it next month.

Thanks for your contribution again! I will continue to update the progress here, and you are welcome to review it.

@KevinnZou
Copy link
Owner

KevinnZou commented Nov 30, 2023

Adding a CefRequestHandler means overriding the default CefRequestHandler.

We had that problem before (user-agent) and we (or I) have to reimplement the default behavior in and make setting a CefRequestHandler extend the default reimplementation instead of overriding.

Additionally this doesn't only break the ``onBeforeBrowse` implementation, it breaks all request handling methods.

I don't have that much time currently as I'm in a release phase right now, best would be to add it directly to KCEF so we don't have to deal with it in this library, maybe you could add a PR if you have time or we have to wait until I have time again (could be anytime in December).

@DatL4g Thanks for your explanation! I have submitted an issue to KCEF. I will try to solve it first and update the progress within that issue.

@KevinnZou
Copy link
Owner

@DatL4g I briefly looked at the implementation of addRequestHandler in KCEF, but I have some confusion.

Adding a CefRequestHandler means overriding the default CefRequestHandler.

We had that problem before (user-agent) and we (or I) have to reimplement the default behavior in and make setting a CefRequestHandler extend the default reimplementation instead of overriding.

Can you provide more detailed instructions for this paragraph? I'm not sure where your default CefRequestHandler is located. Because from the implementation of addRequestHandler, it seems that requestHandler_ is initially null, and this method only saves the handler we provide into requestHandler_ and uses it in onBeforeBrowse. I haven't seen the implementation of the default CefRequestHandler, so I am confused about how can we extend the default reimplementation instead of overriding?

@KevinnZou
Copy link
Owner

@JuBan1 Regarding shouldOverrideUrlLoading not called issue, I searched online but couldn't find a solution. One solution I came up with is to replace shouldOverrideUrlLoading with intercepting shouldInterceptRequest. I tested shouldInterceptRequest and found that it gets called for every request, including internal resource loads on the page. One possible solution is to check request?.isForMainFrame to decide whether to intercept, which should cover most routing interception scenarios. The main commit is here. Do you have any suggestions?

@DatL4g
Copy link
Collaborator

DatL4g commented Dec 3, 2023

@KevinnZou That's right, you have to dig a bit deeper, the CefBrowser_N.java is doing the magic.
If we provide a CefRequestContext which is required for CefRequestHandler the request_context_ variable is set.

The problem here is that this overrides the default behavior/methods since the request_context_ is set to CefRequestContext.getGlobalContext if the request_context_ is null, otherwise this default context is not applied.

So we basically need a extendable RequestContext class which defaults to CefRequestContext.getGlobalContext

Then we can add features like #12 again and much more

@KevinnZou
Copy link
Owner

@DatL4g Thanks for your explanation! I think I understood why we need an extendable RequestContext. However, I'm a bit concerned that I might make mistakes since I'm not very familiar with it. I want to ensure that the code modifications are done accurately. Thus, I would prefer to leave it to you to handle the changes when you have some free time. In the meantime, I will continue working on the JSBridge implementation. Please let me know if there's anything I can do to assist you. Thank you for your contribution again!

@JuBan1
Copy link
Author

JuBan1 commented Dec 6, 2023

The main commit is here. Do you have any suggestions?

I'm not sure if this mechanism makes sense for shouldInterceptRequest. As I understand it, shouldInterceptRequest is called not only for the "main" url but also for content like images, css, etc.

The request interceptor gives the impression that I can Allow/Reject/Redirect this request, but Reject/Redirect changes the webview's main url, not the request that triggered the interceptor.

It seems to me that this is ultimately confusing and not very useful.

I would say it's better to use shouldOverrideUrlLoading and either

  • accept (and document) that the intercepted requests depend on the platform, or
  • limit everything to the lowest common denominator by only intercepting main urls.

@KevinnZou
Copy link
Owner

@JuBan1 Thanks for your suggestions! I understand your concern, shouldOverrideUrlLoading should be the first choice if it can intercept all the URLs. However, it is still unclear why it can only intercept partial requests. Thus, we have to make a choice:

  1. use shouldOverrideUrlLoading but cannot intercept all requests.
  2. use shouldInterceptRequest but only intercept the main frame and use stopLoading to perform the request rejection.

In my opinion, I would prefer the second option. For the first choice, developers are unsure which requests can be intercepted and which cannot, making it unsuitable for production use. The second option is not ideal, but it can at least intercept all requests. What do you think?

@KevinnZou
Copy link
Owner

Adding a CefRequestHandler means overriding the default CefRequestHandler.

We had that problem before (user-agent) and we (or I) have to reimplement the default behavior in and make setting a CefRequestHandler extend the default reimplementation instead of overriding.

Additionally this doesn't only break the ``onBeforeBrowse` implementation, it breaks all request handling methods.

I don't have that much time currently as I'm in a release phase right now, best would be to add it directly to KCEF so we don't have to deal with it in this library, maybe you could add a PR if you have time or we have to wait until I have time again (could be anytime in December).

@DatL4g Are there any updates?

@KevinnZou
Copy link
Owner

@JuBan1 I reported a bug regarding the shouldOverrideUrlLoading not intercepting all requests, and received a reply from the bug tracker. They explained that it is intentional for shouldOverrideUrlLoading to not intercept the initial load request.

Thus, regarding the previous discussion, we may have to make a choice:

  1. use shouldOverrideUrlLoading and make iOS and desktop also not intercept the first load request.
  2. use shouldInterceptRequest to request all the requests.

Do you have any suggestions?

@KevinnZou
Copy link
Owner

Hi all, I have implemented a basic interception in the latest version 1.9.8. Please follow these instructions to conduct a test and let me know if you encounter any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants