Opened 4 years ago

Closed 3 years ago

#17460 closed Cleanup/optimization (fixed)

add KEY to HIDDEN_SETTINGS regexp

Reported by: daenney Owned by: chomik
Component: Core (Other) Version: 1.4-alpha-1
Severity: Normal Keywords: hidden settings, hidden_settings, hide settings
Cc: lpiatek Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

The HIDDEN_SETTINGS regexp is very useful but currently doesn't do much to obfuscate things such as API_KEY settings which are becoming more and more common to hook into additional services.

This would also be solved if/when #16435 gets resolved as it would allow people to add KEY to that regexp themselves.

However, for now and because the usage of such API key's is mainstream, I think it would be a good idea to add KEY to the default HIDDEN_SETTINGS regexp.

Attachments (5)

HIDDEN_SETTINGS.patch (490 bytes) - added by daenney 4 years ago.
django-17460-hidden_settings.patch (509 bytes) - added by bpeschier 4 years ago.
hidden_settingsv2.patch (1.7 KB) - added by daenney 4 years ago.
hidden_settings3.patch (1.6 KB) - added by tomaszrybak 3 years ago.
Fixed doc to be the same as regexp
hidden_settings4.patch (50.7 KB) - added by chomik 3 years ago.
patch with improved documentation and release notes for 1.4-beta-1

Download all attachments as: .zip

Change History (18)

comment:1 Changed 4 years ago by daenney

  • Easy pickings set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 4 years ago by daenney

TOKEN and PASS would be two other fairly common used names for such variables.

Changed 4 years ago by daenney

comment:3 Changed 4 years ago by daenney

  • Has patch set

Added patch.

comment:4 Changed 4 years ago by gabrielhurley

  • Component changed from Uncategorized to Core (Other)
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization

I'm a little loathe to see more work happen on HIDDEN_SETTINGS when it's such an ugly solution to begin with. Especially now that we have custom error reporting. It'd be so much better to see this be part of a common solution to #16435 as noted.

However, those are common sensitive values, so I'll accept the ticket for now.

Changed 4 years ago by bpeschier

comment:5 Changed 4 years ago by bpeschier

  • Triage Stage changed from Accepted to Ready for checkin

Cleaned up the diff so it patches from the root; please remember that the next time you write a patch, it just makes it easier to review the ticket.

I agree with the "should be fixed with a better solution all together", but it does not make the HIDDEN_SETTINGS worse by adding a few good values.

comment:6 Changed 4 years ago by aaugustin

  • Triage Stage changed from Ready for checkin to Accepted

Note that PASS and PASSWORD are redundant in the regexp.

I'm not sure if this needs tests (probably not) or docs (two lines in the "backward-incompatible changes" section of the release notes?).

Last edited 4 years ago by aaugustin (previous) (diff)

comment:7 Changed 4 years ago by bpeschier

  • Patch needs improvement set

Good catch, the docs actually *do* specify which settings get hidden, so updated docs are needed. A mention in the release notes is a good idea as well.

Changed 4 years ago by daenney

comment:8 Changed 4 years ago by daenney

Attached a newer version of the patch that also updates the docs to reflect the changes.

comment:9 Changed 4 years ago by daenney

There's another little issue with the docs, the docs say anything that contains PROFANITIES will match but the HIDDEN_SETTINGS regexp specifies PROFANITIES_LIST as it's pattern so as far as I know PROFANITIES_CAKE won't match in that case.

Changed 3 years ago by tomaszrybak

Fixed doc to be the same as regexp

comment:10 Changed 3 years ago by tomaszrybak

  • Patch needs improvement unset

Now documentation matches regexp HIDDEN_SETTINGS from patch (hidden_settings3.patch).

comment:11 Changed 3 years ago by chomik

  • Owner changed from nobody to chomik

Changed 3 years ago by chomik

patch with improved documentation and release notes for 1.4-beta-1

comment:12 Changed 3 years ago by lpiatek

  • Cc lpiatek added
  • Keywords hidden settings hidden_settings hide settings added
  • Triage Stage changed from Accepted to Ready for checkin

Patch also adds release notes for 1.4-beta-1, which are basicly release notes from 1.4-aplha-1 with aplpha to beta changes where necessary.

I think it is ready for check in.

comment:13 Changed 3 years ago by jezdez

  • Resolution set to fixed
  • Status changed from new to closed

In [17481]:

Fixed #17460 -- Extended the HIDDEN_SETTINGS constant in with a few more sensible names of settings to hide in the debug view. Many thanks to chomik, lpiatek and tomaszrybak.

Note: See TracTickets for help on using tickets.
Back to Top