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 request info for sensitive urls #3175

Closed
wants to merge 1 commit into from

Conversation

jeefy
Copy link
Member

@jeefy jeefy commented Jul 26, 2018

Hey all! This directly addresses #3012 amongst other dupes.

I created an array of URLs that we would deem "sensitive". We would therefore not log any request variables for these URLs. I've tentatively added the two login endpoints.

I also updated the tests to:

  • More easily add additional cases based on URL / Method
  • Ensure those URLs will log "{ contents hidden}" rather than the contents.

Critiques and comments are welcome. :)

Thanks!

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 26, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jeefy
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: cheld

If they are not already assigned, you can assign the PR to them by writing /assign @cheld in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 26, 2018
@codecov
Copy link

codecov bot commented Jul 26, 2018

Codecov Report

Merging #3175 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3175      +/-   ##
==========================================
+ Coverage   54.52%   54.55%   +0.03%     
==========================================
  Files         565      565              
  Lines       12397    12405       +8     
==========================================
+ Hits         6759     6768       +9     
+ Misses       5376     5375       -1     
  Partials      262      262
Impacted Files Coverage Δ
src/app/backend/handler/filter.go 37.64% <100%> (+6.47%) ⬆️
...p/backend/integration/metric/common/aggregation.go 89.23% <0%> (+1.53%) ⬆️

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 6ed35fc...7ffcde3. Read the comment docs.

@jeefy
Copy link
Member Author

jeefy commented Jul 26, 2018

/assign @maciaszczykm

@jeefy
Copy link
Member Author

jeefy commented Jul 27, 2018

Closing in favor of a single PR rather than two staggered PRs.

@jeefy jeefy closed this Jul 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants