Opened 12 years ago

Closed 12 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 12 years ago.
django-17460-hidden_settings.patch (509 bytes ) - added by Bas Peschier 12 years ago.
hidden_settingsv2.patch (1.7 KB ) - added by daenney 12 years ago.
hidden_settings3.patch (1.6 KB ) - added by Tomasz Rybak 12 years ago.
Fixed doc to be the same as regexp
hidden_settings4.patch (50.7 KB ) - added by chomik 12 years ago.
patch with improved documentation and release notes for 1.4-beta-1

Download all attachments as: .zip

Change History (18)

comment:1 by daenney, 12 years ago

Easy pickings: set

comment:2 by daenney, 12 years ago

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

by daenney, 12 years ago

Attachment: HIDDEN_SETTINGS.patch added

comment:3 by daenney, 12 years ago

Has patch: set

Added patch.

comment:4 by Gabriel Hurley, 12 years ago

Component: UncategorizedCore (Other)
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/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.

by Bas Peschier, 12 years ago

comment:5 by Bas Peschier, 12 years ago

Triage Stage: AcceptedReady 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 by Aymeric Augustin, 12 years ago

Triage Stage: Ready for checkinAccepted

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 12 years ago by Aymeric Augustin (previous) (diff)

comment:7 by Bas Peschier, 12 years ago

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.

by daenney, 12 years ago

Attachment: hidden_settingsv2.patch added

comment:8 by daenney, 12 years ago

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

comment:9 by daenney, 12 years ago

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.

by Tomasz Rybak, 12 years ago

Attachment: hidden_settings3.patch added

Fixed doc to be the same as regexp

comment:10 by Tomasz Rybak, 12 years ago

Patch needs improvement: unset

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

comment:11 by chomik, 12 years ago

Owner: changed from nobody to chomik

by chomik, 12 years ago

Attachment: hidden_settings4.patch added

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

comment:12 by lpiatek, 12 years ago

Cc: lpiatek added
Keywords: hidden settings hidden_settings hide settings added
Triage Stage: AcceptedReady 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 by Jannis Leidel, 12 years ago

Resolution: fixed
Status: newclosed

In [17481]:

(The changeset message doesn't reference this ticket)

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