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

Submitter supplied information in commands is unrestricted UTF-8 #398

Closed
dajmaki opened this issue Apr 11, 2019 · 6 comments

Comments

Projects
None yet
5 participants
@dajmaki
Copy link
Contributor

commented Apr 11, 2019

The ledger-api command contains the fields workflow_id, application_id and command_id and places no restrictions on the contents of these strings. Since with the DAML-on-X approach there are going to be multiple different backend implementations that will require correctly handling these
(serializing, storing in database), it would be safer to limit the range of these fields to an ASCII subset (for example as defined by SimpleString).

@meiersi-da

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

Also: we should consider adding length restriction for these fields. Just to be prudent.

dajmaki added a commit that referenced this issue Apr 11, 2019

dajmaki added a commit that referenced this issue Apr 11, 2019

DamlOnX reference implementation (#348)
* Add participant-state and participant-state-index APIs and reference implementations

This adds the (still WIP) interfaces and the in-memory reference implementations
of participant-state and participant-state-index.

See issue #137.

* Pass ledgerId through to getLedgerEnd method

This is needed in the daml-on-x implementation of the
transaction service.

* Add api-server-damlonx and the reference server

This is the initial version of the ledger-api server built on top of
the participant state APIs.

While functionally complete (modulo test services), it has only yet
been tested with the semantic tester. The server and the participant
state APIs are still under active development.

See issue #137.

* Apply scalafmt and copyright headers to new damlonx code

* ledger/damlonx: Fix build errors after merge from master

* ledger/participant-state: separate out the reference impl

* ledger/participant-state-index: separate out reference impl

* ledger/damlonx: Refactoring

- Refactor participant-state into multiple modules
- Introduce structured offset and update id types (vector of ints)
- Properly parse ledger feature flags

* ledger: Disable failing semantic test. Decrease grouping duration.

* scalafmt, fix after merge

* Add missing copyright headers

* ledger/damlonx: Add Simon's comments

* ledger/damlonx: Remove UpdateId, use Offset in both state and state-index.

* ledger/damlonx: scalafmt

* damlonxserver: review participant-state interface

Includes adding issue links for postponed fixes.

* daml-on-x-server: review reference implementation

Fixme's added to #348 and
extra issues creaed in https://github.com/digital-asset/daml/milestone/4

* ledger/damlonx: Use SimpleString

* ledgre/damlonx: Backtrack on SimpleString change

See issue #398

* daml-on-x-server: drop unused 'index.impl.reference.package.scala'

* daml-on-x-server: add note on potential transient contracts bug

* ledger/participant-state*: post-merge fixes
@bitonic

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

@dajmaki @meiersi-da since you are the people most in touch with these alternative backends, could you provide concrete suggestions with what regards to limiting the domain of those ids?

@remyhaemmerle-da

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

TransactionId must be a subset of ContractId since ContractId may be generated by concatenating TrancactionId with an number.

The current specification (daml-lf/spec/value.rst) restricts ContractId to [A-Za-z0-9\._:\-#]+

I proposed to use this for

  • TransactionId
  • CommandId
  • WorkflowId
  • ApplicationId
  • ContractId

We just need a good name for it.

@dajmaki

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

I would suggest adding {1,256}. Enough or we need more?

I think the restriction in the spec for the domain is reasonable. We just want to make sure we avoid any encoding issues with these and that if we do show these we don't allow for any unicode trickery to fool people. The size should be limited to allow printing these in logs and to bound the memory usage.

@remyhaemmerle-da remyhaemmerle-da referenced this issue May 20, 2019

Merged

DAML-LF internal type safety #1192

4 of 5 tasks complete
@meiersi-da

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

Make it {1,255}. This leaves one byte of the 256 for storing the length.

@remyhaemmerle-da

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

All those Ids are now represented by LedgerString.
LedgerString are US-ASCII String with size between 1 and 255 (bounds included) made from digits, letters ., _, :, -, and #

remyhaemmerle-da added a commit that referenced this issue Jun 6, 2019

add forgotten changes to the changelog
about fixed issue #398 and PR #1467

@remyhaemmerle-da remyhaemmerle-da referenced this issue Jun 6, 2019

Merged

add forgotten changes to the changelog #1539

4 of 5 tasks complete

remyhaemmerle-da added a commit that referenced this issue Jun 6, 2019

add forgotten changes to the changelog
about fixed issue #398 and PR #1467

remyhaemmerle-da added a commit that referenced this issue Jun 6, 2019

mergify bot added a commit that referenced this issue Jun 6, 2019

add forgotten changes to the changelog (#1539)
* add forgotten changes to the changelog
about fixed issue #398 and PR #1467

* Address Beth's comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.