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

Validate database prefix against DBNAME_REGEX for system dbs #1647

Merged
merged 1 commit into from Oct 10, 2018

Conversation

Projects
None yet
5 participants
@iilyak
Contributor

iilyak commented Oct 8, 2018

Overview

Previously we only checked that the suffix of the database is
matching one of the predefined system databases. We really should
check the prefix against DBNAME_REGEXP to prevent creation of
illegally named databases.

Testing recommendations

make eunit apps=couch tests=validate_dbname_fail_test_,validate_dbname_success_test_,is_systemdb_test_

Related Issues or Pull Requests

This fixes #1644

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;
@lazedo

This comment has been minimized.

Contributor

lazedo commented Oct 8, 2018

hello,

i just want to make sure that this doesn't break our current usage.
we create DBs with names like this account%2F85%2Fea%2F6075c6c1e266f8512e2233541bdb-201807

@iilyak

This comment has been minimized.

Contributor

iilyak commented Oct 8, 2018

@lazedo

It would still work:

(node1@127.0.0.1)3> couch_db:validate_dbname("account/85/ea/6075c6c1e266f8512e2233541bdb-201807").
ok

@iilyak iilyak force-pushed the cloudant:validate-prefix-for-systemdbs branch 3 times, most recently from eedd250 to 695b633 Oct 8, 2018

@jaydoane

This comment has been minimized.

Contributor

jaydoane commented Oct 9, 2018

make eunit apps=couch tests=validate_dbname_fail_test_,validate_dbname_success_test_,is_systemdb_test_
...
=======================================================
  All 144 tests passed.
{Prefix, true} ->
re:run(Prefix, ?DBNAME_REGEX, [{capture,none}, dollar_endonly])
==
match

This comment has been minimized.

@jaydoane

jaydoane Oct 9, 2018

Contributor

Not sure why == match are not on the same line, but I guess it's not a big deal.

This comment has been minimized.

@iilyak

iilyak Oct 10, 2018

Contributor

I wish we (CouchDB community) have a syntax guidelines. I agree, this formatting is a bit haskellish (or ocamellish). However it provides a good structure visually which helps when reading the code. Although it could be just me because I used to this formatting. Let me know if you want me to change it and which one you would prefer.

  1. operand (new line) operator (new line) operand
re:run(Prefix, ?DBNAME_REGEX, [{capture,none}, dollar_endonly])
==
match
  1. operand (new line) (extra indent) operator operand
re:run(Prefix, ?DBNAME_REGEX, [{capture,none}, dollar_endonly])
    == match
  1. operand (new line) operator operand
re:run(Prefix, ?DBNAME_REGEX, [{capture,none}, dollar_endonly])
== match
  1. extract this into a function match_regexp/1 and use it in other places as well.

This comment has been minimized.

@eiri

eiri Oct 10, 2018

Member

personally I'd split it i two lines

ReOpts =  [{capture,none}, dollar_endonly]
re:run(Prefix, ?DBNAME_REGEX, ReOpts) == match

as it is right now looks more markupish than haskellish to me.

@iilyak iilyak referenced this pull request Oct 10, 2018

Closed

Feature/user partitioned databases #1605

2 of 3 tasks complete
Validate database prefix against DBNAME_REGEX for system dbs
Previously we only checked that the suffix of the database is
matching one of the predefined system databases. We really should
check the prefix against DBNAME_REGEXP to prevent creation of
illegally named databases.

This fixes #1644

@iilyak iilyak force-pushed the cloudant:validate-prefix-for-systemdbs branch from 695b633 to aa63804 Oct 10, 2018

@iilyak iilyak merged commit 9599455 into apache:master Oct 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@iilyak iilyak deleted the cloudant:validate-prefix-for-systemdbs branch Oct 10, 2018

Suffix = filename:basename(Normalized),
case {filename:dirname(Normalized), lists:member(Suffix, ?SYSTEM_DATABASES)} of
{<<".">>, Result} -> Result;
{Prefix, false} -> false;

This comment has been minimized.

@jiangphcn

jiangphcn Oct 25, 2018

Contributor

Just happen to find warning .../db/src/couchdb/src/couch/src/couch_db.erl:1748: Warning: variable 'Prefix' is unused

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment