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

Add all options to UnloadFromSelect, with SQLi protection #27

Merged

Conversation

@graingert
Copy link
Collaborator

@graingert graingert commented Aug 29, 2015

No description provided.

return str(q.compile(dialect=RedshiftDialect(),
compile_kwargs={'literal_binds': True}))

return str(q.compile(

This comment has been minimized.

@graingert

graingert Aug 29, 2015
Author Collaborator

I updated this to match the same helper functions in test_unload_from_select

@graingert graingert force-pushed the add-all-options-to-UnloadFromSelect-with-SQLi-protection branch 3 times, most recently Aug 31, 2015
@graingert
Copy link
Collaborator Author

@graingert graingert commented Aug 31, 2015

@jklukas
jklukas reviewed Aug 31, 2015
View changes
redshift_sqlalchemy/dialect.py Outdated
^aws_access_key_id=[A-Z0-9]{20};
aws_secret_access_key=[A-Za-z0-9/+=]{40}
(?:;token=[A-Za-z0-9/+=]+)?$
""", re.VERBOSE)

This comment has been minimized.

@jklukas

jklukas Aug 31, 2015
Collaborator

Constants should be named in all caps, and I'd prefer RE or REGEX to RX unless this is conforming to a standard I'm not aware of (CREDS_RE).

This comment has been minimized.

@graingert

graingert Aug 31, 2015
Author Collaborator

Done

@jklukas
jklukas reviewed Aug 31, 2015
View changes
redshift_sqlalchemy/dialect.py Outdated
'aws_access_key_id=<access-key-id>;'
'aws_secret_access_key=<secret-access-key>'
'[;token=<temporary-session-token>]\ngot %r' % credentials
)

This comment has been minimized.

@jklukas

jklukas Aug 31, 2015
Collaborator

I'm a little nervous about taking on validation here for an undocumented format.Do we know what error we'd get without this check? Is there significant value in checking here?

This comment has been minimized.

@bouk

bouk Aug 31, 2015
Contributor

I don't really understand why you're formatting something here and then immediately checking whether it's formatted correctly? Can't you just make sure to format it right?

This comment has been minimized.

@cpcloud

cpcloud Aug 31, 2015
Contributor

Do we know what error we'd get without this check?

This is basically the same error message provided by redshift itself.

I don't really understand why you're formatting something here and then immediately checking whether it's formatted correctly?

This is to make sure that something like "'; drop table foo;" isn't passed in

This comment has been minimized.

@cpcloud

cpcloud Aug 31, 2015
Contributor

Alternatively, one could check each parameter against the corresponding regex.

This comment has been minimized.

@graingert

graingert Aug 31, 2015
Author Collaborator

agreed fixing this now

@graingert graingert force-pushed the add-all-options-to-UnloadFromSelect-with-SQLi-protection branch Aug 31, 2015
@jklukas
jklukas reviewed Aug 31, 2015
View changes
redshift_sqlalchemy/dialect.py Outdated
delimiter='DELIMITER AS :delimiter' if el.delimiter is not None else '',
encrypted='ENCRYPTED' if el.encrypted else '',
fixed_width=(
'FIXEDWIDTH AS :fixed_width' if el.fixed_width is not None else ''

This comment has been minimized.

@jklukas

jklukas Aug 31, 2015
Collaborator

Why check None specifically here rather than relying on the boolean interpretation of el.fixed_width? Would we want to omit this clause in the case of an empty list?

This comment has been minimized.

@graingert

graingert Aug 31, 2015
Author Collaborator

None is the default? I think I want it to break accordingly if someone passes an empty list or False. I'm not tied to this.

This comment has been minimized.

@graingert

graingert Aug 31, 2015
Author Collaborator

okay I'm updating fixed_width, note null and delimiter must be is not None or we can't support the empty string null or delimiter value

@@ -16,7 +16,7 @@ deps =
commands=flake8 redshift_sqlalchemy tests

[flake8]
max-line-length = 128
max-line-length = 89

This comment has been minimized.

@jklukas

jklukas Aug 31, 2015
Collaborator

Any reason not to set this to 80?

This comment has been minimized.

@graingert

graingert Aug 31, 2015
Author Collaborator

I'm working on removing this line altogether. This PR is part of that work.

@graingert graingert force-pushed the add-all-options-to-UnloadFromSelect-with-SQLi-protection branch Aug 31, 2015
@jklukas
Copy link
Collaborator

@jklukas jklukas commented Aug 31, 2015

This is looking much nicer for the refactor. I like this PR.

@graingert graingert force-pushed the add-all-options-to-UnloadFromSelect-with-SQLi-protection branch 5 times, most recently Aug 31, 2015
validate credentials before format

Better exception text

prevent an empty FIXEDWITH string as that's an error.
@graingert graingert force-pushed the add-all-options-to-UnloadFromSelect-with-SQLi-protection branch to cdcd103 Aug 31, 2015
graingert added a commit that referenced this pull request Aug 31, 2015
…lect-with-SQLi-protection

Add all options to UnloadFromSelect, with SQLi protection
@graingert graingert merged commit b48dbe7 into master Aug 31, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@graingert graingert deleted the add-all-options-to-UnloadFromSelect-with-SQLi-protection branch Aug 31, 2015
haleemur pushed a commit to haleemur/redshift_sqlalchemy that referenced this pull request Sep 2, 2015
…ons-to-UnloadFromSelect-with-SQLi-protection

Add all options to UnloadFromSelect, with SQLi protection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.