Opened 8 years ago

Closed 5 years ago

#5192 closed (fixed)

Flatpages should be able to handle more characters (e.g. ~)

Reported by: marco.giusti@… Owned by: ctrochalakis
Component: Contrib apps Version: master
Severity: Keywords:
Cc: yatiohi@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

url validator is wrong and flatpages uses core.isAlphaNumericUrl in his model. expecially chars like `~' are not accepted.

Attachments (4)

5192a.diff (1007 bytes) - added by simeon 7 years ago.
Alter django.core.validators.py
5192b.diff (653 bytes) - added by simeon 7 years ago.
Alter django/contrib/flatpages/models.py
0001-contrib.flatpages-URL-can-now-contain-tilde.patch (1.5 KB) - added by ctrochalakis 7 years ago.
tilde-v1
0001-add-support-for-dots-and-tildes-in-flatpages.patch (1.2 KB) - added by ctrochalakis 6 years ago.
+dots+tildes, new patch on top of trunk

Download all attachments as: .zip

Change History (24)

comment:1 follow-up: Changed 8 years ago by Simon G. <dev@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

How is the url validator wrong? is it just the flatpages app that you're having issues with? can you give some details please?

comment:2 in reply to: ↑ 1 Changed 8 years ago by anonymous

  • Resolution invalid deleted
  • Status changed from closed to reopened

Replying to Simon G. <dev@simon.net.nz>:

How is the url validator wrong? is it just the flatpages app that you're having issues with? can you give some details please?

this is the re used to validate urls:

url_re = re.compile(r'^https?://\S+$')

but urls can have different schemas, addictionally urls have some restrictions for unsafe chars (rfc1738).

also working whid unquoted chars instead of quoted unsafe ones it's a good thing for url dispatch sanity.

flatpages uses the following regex to check urls:

alnumurl_re = re.compile(r'^[-\w/]+$')

so how can i use unsafe (quoted) chars like tilde `~'? in rfc2396, which revises and replaces rfc1738, adds tilde to unreserved characters "...since it is extensively used on the Internet in spite of the difficulty to transcribe it with some keyboards."

comment:3 Changed 7 years ago by Simon G <dev@…>

  • Component changed from Validators to Contrib apps
  • Summary changed from wrong url validator to Flatpages should be able to handle more characters (e.g. ~)
  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 7 years ago by simeon

  • Has patch set
  • Owner changed from nobody to simeon
  • Status changed from reopened to new

Simon G: Are we wanting to change the isAlphaNumericURL validator in django/core/validators.py to also accept a tilde char? If so see patch 5192a.diff... You've changed the component and summary which leads me to think that you don't want to and would rather isolate the changes to the flatpages module (although I'd argue that a url filter should allow tildes. Of course than we have to update the translations of the validation error message). If you'd rather just touch the flatpages module and not worry about the slightly misleading error message we could monkeypatch the appropriate validators re (see 5192b.diff) and call it good.

Changed 7 years ago by simeon

Alter django.core.validators.py

Changed 7 years ago by simeon

Alter django/contrib/flatpages/models.py

comment:5 Changed 7 years ago by Simon G <dev@…>

  • Triage Stage changed from Accepted to Ready for checkin

Hmm.. good question. We should probably do what the RFC's say, but that involves modifying isAlphaNumericURL into something that's isAlphaNumericURLAndSomeExtraThingsLikeTilde.

I'll offload it to one of the core developers to decide what's best :)

comment:6 Changed 7 years ago by marco giusti <marco.giusti@…>

maybe isRFC2396Compliant is a good choice or simply isValidUrl

comment:7 Changed 7 years ago by jacob

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

This is the wrong change: it makes the alpha-numeric regular expression match "~". The right way to change this is by modifying the url field on FlatPage itself.

comment:8 Changed 7 years ago by ctrochalakis

  • Owner changed from simeon to ctrochalakis
  • Status changed from new to assigned

Jacob, do you mean using a RegexField for example?

comment:9 Changed 7 years ago by ctrochalakis

Oops, sorry for that. RegexField is newforms not models.fields!

Is the correct way creating a new validator and appending it to validator_list?

Changed 7 years ago by ctrochalakis

tilde-v1

comment:10 Changed 7 years ago by ctrochalakis

  • Patch needs improvement unset

Modified the URL field on flatpage application. (tilde-v1 patch)

comment:11 Changed 7 years ago by Kevin McCarthy <me@…>

Also: attempting to add a url like /test.html as a flatpage is rejected with the following error:

  • This value must contain only letters, numbers, underscores, dashes or slashes.

I suspect it is the . (period), although the lack of trailing slash might be throwing it for a loop as well?

comment:12 Changed 7 years ago by jacob

#7663 was a duplicate.

comment:13 Changed 7 years ago by idangazit

Following conversation with malcolmt on IRC, leaving this alone for now and removing it from SprintIdeas as it is post-1.0.

I think the question here is how to validate the URLs entered for flatpages now that oldforms/validation isn't around anymore. It can go one of two ways:

  • A simplistic regex that tries to conform to "average case" usage of flatpages, coupled with docs saying what URLs are supported in flatpages. Benefits: short(er) regex, easier to maintain, secure, and debug. Downsides: people creating flatpages in unusual locations will run into trouble.
  • RFC2396-compliant regex. Benefits: flatpages can work with any valid URL, "standards compliance". Downsides: monsterously complex regex, difficult to secure, difficult to understand and maintain.

Hope this helps focus future work on this ticket!

comment:14 Changed 7 years ago by idangazit

  • milestone set to post-1.0

Correction, didn't remove from sprintideas, but retargeted for post-1.0 and noted as such in SprintIdeas.

comment:15 Changed 7 years ago by nealmcb

I ran into the same problem while trying to verify my site to google's "webmaster tools". They tell you the url to create and you have to create it to prove that you control the site.
They asked for a random url ending with ".html" (google29988dbab2fb15d3.html), which generated the error noted above from flatpages.

So certainly adding "." to the legal characters sounds to me like a good and easy idea.

Or can the validation step be made optional so the user can override it?

comment:16 Changed 6 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:17 Changed 6 years ago by bugmenot

It is rather old ticket and it annoys sometimes. May be we can use this way for now?

A simplistic regex that tries to conform to "average case" usage of flatpages, coupled with docs saying
what URLs are supported in flatpages. Benefits: short(er) regex, easier to maintain, secure, and debug.
Downsides: people creating flatpages in unusual locations will run into trouble.

Just add some common symbols ("~", ".", may be some more) to the regexp. It should be easy and should not make troubles for anybody. And someday, where it will be time, think about more powerful regexp, may be checking all RFC2396-valid URLs.

Changed 6 years ago by ctrochalakis

+dots+tildes, new patch on top of trunk

comment:18 Changed 6 years ago by ctrochalakis

  • Cc yatiohi@… added

The old patch couldn't be applied anymore. Now the validation code is located in admin.py, I wrote a new patch adding support for both dots(.) and tildes(~)

comment:19 Changed 6 years ago by SmileyChris

  • Triage Stage changed from Accepted to Ready for checkin

comment:20 Changed 5 years ago by russellm

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

(In [13655]) Fixed #5192 -- Modified flatpage admin form to allow ~ and . characters in flatpage names. Thanks to marco.giusti@… for the report, Idan Gazit for summarizing the issue on the ticket, and ctrochalakis for the initial patch.

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