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

Hide api key in xhr-responses if key is configured in config.php #454

Merged
merged 2 commits into from
Feb 21, 2019

Conversation

IljaN
Copy link
Member

@IljaN IljaN commented Feb 21, 2019

If the api-key is configured via config.php the api-key is still leaked via XHR-Responses. This PR hides the API-Key in this case. Contrary to assumptions made in #453 the frontend does not need any changes as it seems to be able to handle the missing apiKey-field without errors.

Fixes #453

@IljaN IljaN self-assigned this Feb 21, 2019
@IljaN IljaN added this to the development milestone Feb 21, 2019
@codecov
Copy link

codecov bot commented Feb 21, 2019

Codecov Report

Merging #454 into master will increase coverage by 2.95%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #454      +/-   ##
============================================
+ Coverage     53.17%   56.12%   +2.95%     
- Complexity      268      269       +1     
============================================
  Files            17       17              
  Lines           961      930      -31     
============================================
+ Hits            511      522      +11     
+ Misses          450      408      -42
Impacted Files Coverage Δ Complexity Δ
lib/Controller/LocalAppsController.php 0% <ø> (ø) 5 <0> (ø) ⬇️
lib/Controller/MarketController.php 6.91% <100%> (+6.91%) 42 <0> (+1) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3128a70...a8eec47. Read the comment docs.

@IljaN IljaN requested a review from PVince81 February 21, 2019 14:05
@IljaN IljaN changed the title Hide api key in xhr-requests if key is configured in config.php Hide api key in xhr-responses if key is configured in config.php Feb 21, 2019
@PVince81
Copy link
Contributor

@IljaN no UI changes needed ?

would be good to add a bit more context when writing PRs, make sure to think about who will review it and what questions they'll have

@IljaN
Copy link
Member Author

IljaN commented Feb 21, 2019

@PVince81 No, the existing frontend code seems to handle it pretty well. User is shown as Logged-In and change-api-key button is hidden.

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@IljaN
Copy link
Member Author

IljaN commented Feb 21, 2019

Updated PR description

@PVince81 PVince81 merged commit 3753903 into master Feb 21, 2019
@delete-merged-branch delete-merged-branch bot deleted the dont_leak_apikey branch February 21, 2019 16:18
@PVince81 PVince81 mentioned this pull request Feb 21, 2019
26 tasks
@PVince81 PVince81 modified the milestones: development, QA Feb 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants