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

Support Basic Authentication on Content-Type: text/plain pages #7891

Closed
wants to merge 1 commit into from

Conversation

mrtc0
Copy link

@mrtc0 mrtc0 commented Jul 20, 2023

Purpose

Fix issue Basic Authentication fails on pages with Content-Type: text/plain.
As a use case, if a proxy in front of the application is responsible for Basic Authentication, proxy may return a text/plain response.

Approach

Even on a page with Content-Type: text/plain, the _tryAuthorizeWithHttpBasicAuthCredentials function will be called to authorize Basic Authentication.

References

N/A

Pre-Merge TODO

  • Write tests for your proposed changes
  • Make sure that existing tests do not fail

Signed-off-by: Kohei Morita <moritakouhei@graffer.jp>
@need-response-app need-response-app bot added the STATE: Need response An issue that requires a response or attention from the team. label Jul 20, 2023
@mrtc0 mrtc0 temporarily deployed to authentication July 20, 2023 04:31 — with GitHub Actions Inactive
@mrtc0 mrtc0 marked this pull request as ready for review July 20, 2023 04:32
@mrtc0 mrtc0 changed the title Support basic authentication on Content-Type: text/plain pages Support Basic Authentication on Content-Type: text/plain pages Jul 20, 2023
@mrtc0 mrtc0 temporarily deployed to CI July 24, 2023 07:18 — with GitHub Actions Inactive
@AlexKamaev
Copy link
Contributor

At present, TestCafe cannot open text pages in Native Automation mode: #7786
Why do you need to open a text page in Native Automation mode? Please describe your usage scenario in detail.

@need-response-app need-response-app bot removed the STATE: Need response An issue that requires a response or attention from the team. label Jul 24, 2023
@AlexKamaev AlexKamaev added the STATE: Need clarification An issue lacks information for further research. label Jul 24, 2023
@mrtc0
Copy link
Author

mrtc0 commented Jul 24, 2023

@AlexKamaev The test target page is served with Content-Type: text/html, but there might be cases where the response for the initial challenge of Basic authentication returns with Content-Type: text/plain.

In my case, the Basic authentication component implemented using Go's net/http was returning a first challenge's response with Content-Type: text/plain and a status code of 401.
This can be fixed by modifying the response to return with Content-Type: text/html, However, there might be situations where such modifications are not always controllable or feasible due to various constraints or limitations in the environment or the system.

@need-response-app need-response-app bot added the STATE: Need response An issue that requires a response or attention from the team. label Jul 24, 2023
@github-actions github-actions bot removed the STATE: Need clarification An issue lacks information for further research. label Jul 24, 2023
@AlexKamaev
Copy link
Contributor

In this case, the described issue is the same as #7786. We need to support injecting TestCafe scripts in the test page. You can try modifying the ContentType response header as you mentioned.

Please also remove the duplicated code:

const contentType = responseHeaders
?.find(header => header.name.toLowerCase() === 'content-type')
?.value;

Moreover, I think it will be enough to write a test for a text page without HttpAuth.

@need-response-app need-response-app bot removed the STATE: Need response An issue that requires a response or attention from the team. label Jul 25, 2023
@AlexKamaev AlexKamaev added the STATE: Need clarification An issue lacks information for further research. label Jul 25, 2023
@AlexKamaev
Copy link
Contributor

I've closed this PR since there was no activity for a long time

@AlexKamaev AlexKamaev closed this Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
STATE: Need clarification An issue lacks information for further research.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants