Skip to content
This repository was archived by the owner on Feb 25, 2022. It is now read-only.

Conversation

@tripodsan
Copy link
Contributor

@tripodsan tripodsan commented Aug 26, 2020

fixes #118

changes

  • the sheet inside an excel workbook or google spreadsheet can now be addressed using the sheet parameter.
  • only sheets having the helix- prefix can be addressed
  • if the workbook or spreadsheet does not have any helix- prefixed sheets, the first one is returned.
  • By default the used ranged of the selected sheet is returned.
  • For excel, A table can be addressed using the table request parameter, which can be a table name or an index. For example, table=Table1 will return the table with the name Table1, table=1 will return the second table in the sheet.
  • Note that specifying a table index will result in an additional request to lookup the table names.

@github-actions
Copy link

github-actions bot commented Aug 26, 2020

This PR will trigger a minor release when merged.

@codecov
Copy link

codecov bot commented Aug 26, 2020

Codecov Report

Merging #162 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #162   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           11        11           
  Lines          292       320   +28     
=========================================
+ Hits           292       320   +28     
Impacted Files Coverage Δ
src/matchers/excel.js 100.00% <100.00%> (ø)
src/matchers/google.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a120f29...d2b2f54. Read the comment docs.

Copy link
Contributor

@trieloff trieloff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have to break compatibility, let's make sure we update Google and Excel at the same time.

Overall, I'd much prefer a non-breaking change.

@tripodsan
Copy link
Contributor Author

If we have to break compatibility, let's make sure we update Google and Excel at the same time.

ok.

Overall, I'd much prefer a non-breaking change.

yes, we could serve the first sheet, if there is no helix-default. but this is one of the security problems we want to fix.
otherwise, you can expose any excel workbook, even the ones not meant for web.

@trieloff
Copy link
Contributor

yes, we could serve the first sheet, if there is no helix-default. but this is one of the security problems we want to fix.
otherwise, you can expose any excel workbook, even the ones not meant for web.

A more gentle change would be to use the new behavior whenever there is a sheet called helix-* and serve the first sheet otherwise.

@tripodsan
Copy link
Contributor Author

tripodsan commented Aug 26, 2020

yes, we could serve the first sheet, if there is no helix-default. but this is one of the security problems we want to fix.
otherwise, you can expose any excel workbook, even the ones not meant for web.

A more gentle change would be to use the new behavior whenever there is a sheet called helix-* and serve the first sheet otherwise.

exactly not. otherwise you can expose any excel workbook that does not have an helix-* sheet in it.

@trieloff
Copy link
Contributor

You can only expose the first sheet. I wouldn't allow the table parameter at all. And this only applies to workbooks that have been shared with Helix in the first place, so I don't see how this would result in additional disclosure.

@tripodsan
Copy link
Contributor Author

You can only expose the first sheet.

currently, yes. you can only expose the first sheet.
with this enhancement, you can also expose other sheets. as requested in the issue.

I wouldn't allow the table parameter at all.

that would be another breaking change. in case someone really wants to share a specific table in a sheet, I think this is useful.

And this only applies to workbooks that have been shared with Helix in the first place, so I don't see how this would result in additional disclosure.

yes - but, people have all sorts of workbooks in their drive that they might expose accidentally. so better safe than sorry.

@tripodsan tripodsan changed the title feat(excel): add sheet addressing feat(data): add sheet addressing Aug 26, 2020
@tripodsan
Copy link
Contributor Author

A more gentle change would be to use the new behavior whenever there is a sheet called helix-* and serve the first sheet otherwise.

After discussion with @davidnuescheler we decided to go with this idea.

Co-authored-by: Lars Trieloff <lars@trieloff.net>
@tripodsan tripodsan merged commit 59ed953 into master Aug 27, 2020
@tripodsan tripodsan deleted the sheet-adressing branch August 27, 2020 10:16
trieloff pushed a commit that referenced this pull request Aug 27, 2020
# [1.9.0](v1.8.16...v1.9.0) (2020-08-27)

### Features

* **data:** add sheet addressing ([#162](#162)) ([59ed953](59ed953)), closes [#118](#118)
@trieloff
Copy link
Contributor

🎉 This PR is included in version 1.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

support addressing excel sheets

3 participants